[PATCH linux dev 6.11] hwmon:add new hwmon driver sq52205

[PATCH linux dev 6.11] hwmon:add new hwmon driver sq52205

[Wenliang]

Thank you for bringing up your questions and suggestions.The SQ52205 is a
part number specific to the Asian region, which is why you might not be
able to find it through a search. I'll provide you the website
(https://us1.silergy.com/zh/productsview/SQ52205FBP).
Some registers of this chip are similar to those of the INA226, but it has
additional registers such as integrators, which is the main reason why I'm
offering a new driver.And I plan add drivers of the same series based on
this. I commit the new patch and look forward to your reply.

Signed-off-by: Wenliang <wenliang202407@163.com>

---
 Documentation/hwmon/index.rst   |   1 +
 Documentation/hwmon/sq52205.rst |  44 +++
 MAINTAINERS                     |   5 +
 drivers/hwmon/Kconfig           |  13 +
 drivers/hwmon/Makefile          |   1 +
 drivers/hwmon/sq52205.c         | 558 ++++++++++++++++++++++++++++++++
 6 files changed, 622 insertions(+)
 create mode 100644 Documentation/hwmon/sq52205.rst
 create mode 100644 drivers/hwmon/sq52205.c

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 913c11390a45..5130270c8efe 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -219,6 +219,7 @@ Hardware Monitoring Kernel Drivers
    smsc47m1
    sparx5-temp
    spd5118
+   sq52205
    stpddc60
    surface_fan
    sy7636a-hwmon
diff --git a/Documentation/hwmon/sq52205.rst b/Documentation/hwmon/sq52205.rst
new file mode 100644
index 000000000000..e66bf883b42b
--- /dev/null
+++ b/Documentation/hwmon/sq52205.rst
@@ -0,0 +1,44 @@
+SPDX-License-Identifier: GPL-2.0-only
+
+Kernel driver sq52205
+====================
+
+Supported chips:
+
+  * Silergy SQ52205
+
+
+    Prefix: 'sq52205'
+    Addresses: I2C 0x40 - 0x4f
+
+    Datasheet: Publicly available at the Silergy website
+
+	       https://us1.silergy.com/zh
+
+
+Author: Wenliang Yan <wenliang202407@163.com>
+
+Description
+-----------
+
+The SQ52205 is a high- and low-side current shunt and power monitor with an I2C
+interface. The SQ52205 both shunt drop and supply voltage, with programmable
+calibration value and conversion times. The SQ52205 can also calculate average
+power for use in energy conversion.
+
+
+
+Sysfs entries
+---------------------
+
+======================= ===============================
+in0_input		Shunt voltage(mV) channel
+in1_input		Bus voltage(mV) channel
+curr1_input		Current(mA) measurement channel
+power1_input		Power(uW) measurement channel
+shunt_resistor		Shunt resistance(uOhm) channel
+update_interval		data conversion time; affects number of samples used
+			to average results for shunt and bus voltages.
+calculate_avg_power	calculate average power from last reading to the present
+======================= ===============================
+
diff --git a/MAINTAINERS b/MAINTAINERS
index 8766f3e5e87e..4794c703da34 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20822,6 +20822,11 @@ S:	Maintained
 F:	drivers/input/touchscreen/silead.c
 F:	drivers/platform/x86/touchscreen_dmi.c
 
+SILERGY HARDWARE MONITOR DRIVER
+M:	Wenliang Yan <wenliang202407@163.com>
+S:	Maintained
+F:	drivers/hwmon/sq52205.c
+
 SILICON LABS WIRELESS DRIVERS (for WFxxx series)
 M:	Jérôme Pouiller <jerome.pouiller@silabs.com>
 S:	Supported
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index b60fe2e58ad6..7a6c21c9eee9 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2521,6 +2521,19 @@ config SENSORS_INTEL_M10_BMC_HWMON
 	  sensors monitor various telemetry data of different components on the
 	  card, e.g. board temperature, FPGA core temperature/voltage/current.
 
+config SENSORS_SQ52205
+	tristate "Silergy SQ52205 and compatibles"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  If you say yes here you get support for SQ52205 power monitor chips.
+
+	  The SQ52205 driver is configured for the default configuration of
+	  the part as described in the datasheet.
+	  Default value for Rshunt is 10 mOhms.
+	  This driver can also be built as a module. If so, the module
+	  will be called sq52205.
+
 if ACPI
 
 comment "ACPI drivers"
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index b1c7056c37db..270f88e3c6e6 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -207,6 +207,7 @@ obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
 obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
 obj-$(CONFIG_SENSORS_SPARX5)	+= sparx5-temp.o
 obj-$(CONFIG_SENSORS_SPD5118)	+= spd5118.o
+obj-$(CONFIG_SENSORS_SQ52205)	+= sq52205.o
 obj-$(CONFIG_SENSORS_STTS751)	+= stts751.o
 obj-$(CONFIG_SENSORS_SURFACE_FAN)+= surface_fan.o
 obj-$(CONFIG_SENSORS_SY7636A)	+= sy7636a-hwmon.o
diff --git a/drivers/hwmon/sq52205.c b/drivers/hwmon/sq52205.c
new file mode 100644
index 000000000000..a7a674a50289
--- /dev/null
+++ b/drivers/hwmon/sq52205.c
@@ -0,0 +1,558 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for SQ52205 power monitor chip
+ *
+ * Copyright (C) 2024 Wenliang Yan <wenliang202407@163.com>
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/jiffies.h>
+#include <linux/of.h>
+#include <linux/delay.h>
+#include <linux/util_macros.h>
+#include <linux/regmap.h>
+
+
+
+/* common register definitions */
+#define SQ522XX_CONFIG			0x00
+#define SQ522XX_SHUNT_VOLTAGE	0x01 /* readonly */
+#define SQ522XX_BUS_VOLTAGE		0x02 /* readonly */
+#define SQ522XX_POWER			0x03 /* readonly */
+#define SQ522XX_CURRENT			0x04 /* readonly */
+#define SQ522XX_CALIBRATION		0x05
+
+/* SQ52205 register definitions */
+#define SQ52205_MASK_ENABLE		0x06
+#define SQ52205_ALERT_LIMIT		0x07
+#define SQ52205_EIN				0x0A
+#define SQ52205_ACCUM_CONFIG	0x0D
+
+/* register count */
+#define SQ52205_REGISTERS		0x0D
+#define SQ522XX_MAX_REGISTERS	0x0D
+
+/* settings - default */
+#define SQ52205_CONFIG_DEFAULT		0x4527	/* averages=16 */
+#define SQ52205_ACCUM_CONFIG_DEFAULT	0x044C
+
+/* worst case is 68.10 ms (~14.6Hz) */
+#define SQ522XX_CONVERSION_RATE		15
+#define SQ522XX_MAX_DELAY		69 /* worst case delay in ms */
+
+#define SQ522XX_RSHUNT_DEFAULT		10000	//10mOhms
+#define SQ52205_BUS_VOLTAGE_LSB		1250	//1.25mV
+
+/* bit mask for reading the averaging setting in the configuration register */
+#define SQ52205_AVG_RD_MASK		0x0E00
+
+#define SQ52205_READ_AVG(reg)		(((reg) & SQ52205_AVG_RD_MASK) >> 9)
+#define SQ52205_SHIFT_AVG(val)		((val) << 9)
+
+/* common attrs, sq52205 attrs and NULL */
+#define SQ522XX_MAX_ATTRIBUTE_GROUPS	3
+
+/*
+ * Both bus voltage and shunt voltage conversion times for sq52205 are set
+ * to 0b0100 on POR, which translates to 2200 microseconds in total.
+ */
+#define SQ52205_TOTAL_CONV_TIME_DEFAULT	2200
+
+static const struct regmap_config sq522xx_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+};
+
+enum sq522xx_ids { sq52205 };
+
+struct sq522xx_config {
+	u16 config_default;
+	int calibration_factor;
+	int registers;
+	int shunt_div;
+	int bus_voltage_shift;
+	int bus_voltage_lsb;	/* uV */
+	int power_lsb;			/* uW */
+};
+
+struct sq522xx_data {
+	const struct sq522xx_config *config;
+
+	long rshunt;
+
+	struct mutex config_lock;
+	struct i2c_client *client;
+	struct regmap *regmap;
+
+	const struct attribute_group *groups[SQ522XX_MAX_ATTRIBUTE_GROUPS];
+};
+
+static const struct sq522xx_config sq522xx_config[] = {
+	[sq52205] = {
+		.config_default = SQ52205_CONFIG_DEFAULT,
+		.calibration_factor = 5120000,
+		.registers = SQ52205_REGISTERS,
+		.shunt_div = 400,
+		.bus_voltage_shift = 0,
+		.bus_voltage_lsb = SQ52205_BUS_VOLTAGE_LSB,
+		.power_lsb = 25000,
+	},
+};
+
+/*
+ * Available averaging rates for sq52205. The indices correspond with
+ * the bit values expected by the chip (according to the sq52205 datasheet)
+ */
+static const int sq52205_avg_tab[] = { 1, 4, 16, 64, 128, 256, 512, 1024 };
+
+static int sq52205_reg_to_interval(u16 config)
+{
+	int avg = sq52205_avg_tab[SQ52205_READ_AVG(config)];
+
+	/*
+	 * Multiply the total conversion time by the number of averages.
+	 * Return the result in milliseconds.
+	 */
+	return DIV_ROUND_CLOSEST(avg * SQ52205_TOTAL_CONV_TIME_DEFAULT, 1000);
+}
+
+/*
+ * Return the new, shifted AVG field value of CONFIG register,
+ * to use with regmap_update_bits
+ */
+static u16 sq52205_interval_to_reg(int interval)
+{
+	int avg, avg_bits;
+
+	avg = DIV_ROUND_CLOSEST(interval * 1000,
+				SQ52205_TOTAL_CONV_TIME_DEFAULT);
+	avg_bits = find_closest(avg, sq52205_avg_tab,
+				ARRAY_SIZE(sq52205_avg_tab));
+
+	return SQ52205_SHIFT_AVG(avg_bits);
+}
+
+/*
+ * Calibration register is set to the best value, which eliminates
+ * truncation errors on calculating current register in hardware.
+ * According to datasheet the best values are 2048 for
+ * sq52205. They are hardcoded as calibration_value.
+ */
+static int sq522xx_calibrate(struct sq522xx_data *data)
+{
+	u16 val = DIV_ROUND_CLOSEST(data->config->calibration_factor,
+					data->rshunt);
+
+	return regmap_write(data->regmap, SQ522XX_CALIBRATION,
+				val);
+}
+
+/*
+ * Initialize the configuration and calibration registers.
+ */
+static int sq522xx_init(struct sq522xx_data *data)
+{
+	int ret = regmap_write(data->regmap, SQ522XX_CONFIG,
+				data->config->config_default);
+	if (ret < 0)
+		return ret;
+
+	return sq522xx_calibrate(data);
+}
+static int sq52205_init(struct sq522xx_data *data)
+{
+	// configure ENABLE register
+	int ret = regmap_write(data->regmap, SQ52205_ACCUM_CONFIG,
+				SQ52205_ACCUM_CONFIG_DEFAULT);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int sq522xx_read_reg(struct device *dev, int reg, unsigned int *regval)
+{
+	struct sq522xx_data *data = dev_get_drvdata(dev);
+	int ret, retry;
+
+	dev_dbg(dev, "Starting register %d read\n", reg);
+
+	for (retry = 5; retry; retry--) {
+
+		ret = regmap_read(data->regmap, reg, regval);
+		if (ret < 0)
+			return ret;
+
+		dev_dbg(dev, "read %d, val = 0x%04x\n", reg, *regval);
+
+		/*
+		 * If the current value in the calibration register is 0, the
+		 * power and current registers will also remain at 0. In case
+		 * the chip has been reset let's check the calibration
+		 * register and reinitialize if needed.
+		 * We do that extra read of the calibration register if there
+		 * is some hint of a chip reset.
+		 */
+		if (*regval == 0) {
+			unsigned int cal;
+
+			ret = regmap_read(data->regmap, SQ522XX_CALIBRATION,
+					  &cal);
+			if (ret < 0)
+				return ret;
+
+			if (cal == 0) {
+				dev_warn(dev, "chip not calibrated, reinitializing\n");
+
+				ret = sq522xx_init(data);
+				if (ret < 0)
+					return ret;
+				/*
+				 * Let's make sure the power and current
+				 * registers have been updated before trying
+				 * again.
+				 */
+				msleep(SQ522XX_MAX_DELAY);
+				continue;
+			}
+		}
+		return 0;
+	}
+
+	/*
+	 * If we're here then although all write operations succeeded, the
+	 * chip still returns 0 in the calibration register. Nothing more we
+	 * can do here.
+	 */
+	dev_err(dev, "unable to reinitialize the chip\n");
+	return -ENODEV;
+}
+
+static int sq522xx_get_value(struct sq522xx_data *data, u8 reg,
+				unsigned int regval)
+{
+	int val;
+
+	switch (reg) {
+	case SQ522XX_SHUNT_VOLTAGE:
+		/* signed register , value is in mV*/
+		val = DIV_ROUND_CLOSEST((s16)regval, data->config->shunt_div);
+		break;
+	case SQ522XX_BUS_VOLTAGE:
+		/* signed register , value is in mV*/
+		val = (regval >> data->config->bus_voltage_shift)
+		* data->config->bus_voltage_lsb;
+		val = DIV_ROUND_CLOSEST(val, 1000);
+		break;
+	case SQ522XX_POWER:
+		/* value is in uV*/
+		val = regval * data->config->power_lsb;
+		break;
+	case SQ522XX_CURRENT:
+		/* signed register, LSB=1mA (selected), in mA */
+		val = (s16)regval;
+		break;
+	case SQ522XX_CALIBRATION:
+		val = DIV_ROUND_CLOSEST(data->config->calibration_factor,
+					regval);
+		break;
+	default:
+		/* programmer goofed */
+		WARN_ON_ONCE(1);
+		val = 0;
+		break;
+	}
+
+	return val;
+}
+
+static ssize_t sq522xx_value_show(struct device *dev,
+				 struct device_attribute *da, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
+	struct sq522xx_data *data = dev_get_drvdata(dev);
+	unsigned int regval;
+
+	int err = sq522xx_read_reg(dev, attr->index, &regval);
+
+	if (err < 0)
+		return err;
+
+	return sysfs_emit(buf, "%d\n", sq522xx_get_value(data, attr->index, regval));
+}
+
+
+/*
+ * In order to keep calibration register value fixed, the product
+ * of current_lsb and shunt_resistor should also be fixed and equal
+ * to shunt_voltage_lsb = 1 / shunt_div multiplied by 10^9 in order
+ * to keep the scale.
+ */
+static ssize_t sq522xx_shunt_store(struct device *dev,
+				  struct device_attribute *da,
+				  const char *buf, size_t count)
+{
+	unsigned long val;
+	int status;
+	struct sq522xx_data *data = dev_get_drvdata(dev);
+
+	status = kstrtoul(buf, 10, &val);
+	if (status < 0)
+		return status;
+	/* Values greater than the calibration factor make no sense. */
+	if (val == 0 || val > data->config->calibration_factor)
+		return -EINVAL;
+
+	mutex_lock(&data->config_lock);
+	data->rshunt = val;
+
+	status = sq522xx_calibrate(data);
+	mutex_unlock(&data->config_lock);
+	if (status < 0)
+		return status;
+
+	return count;
+}
+
+static ssize_t sq522xx_shunt_show(struct device *dev,
+				 struct device_attribute *da, char *buf)
+{
+	struct sq522xx_data *data = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%li\n", data->rshunt);
+}
+
+
+
+static ssize_t sq52205_interval_store(struct device *dev,
+					struct device_attribute *da,
+					const char *buf, size_t count)
+{
+	struct sq522xx_data *data = dev_get_drvdata(dev);
+	unsigned long val;
+	int status;
+
+	status = kstrtoul(buf, 10, &val);
+	if (status < 0)
+		return status;
+
+	if (val > INT_MAX || val == 0)
+		return -EINVAL;
+
+	status = regmap_update_bits(data->regmap, SQ522XX_CONFIG,
+					SQ52205_AVG_RD_MASK,
+					sq52205_interval_to_reg(val));
+	if (status < 0)
+		return status;
+
+	return count;
+}
+
+static ssize_t sq52205_interval_show(struct device *dev,
+					struct device_attribute *da, char *buf)
+{
+	struct sq522xx_data *data = dev_get_drvdata(dev);
+	int status;
+	unsigned int regval;
+
+	status = regmap_read(data->regmap, SQ522XX_CONFIG, &regval);
+	if (status)
+		return status;
+
+	return sysfs_emit(buf, "%d\n", sq52205_reg_to_interval(regval));
+}
+
+static int sq52205_read_reg48(const struct i2c_client *client, u8 reg,
+					long *accumulator_24, long *sample_count)
+{
+	u8 data[6];
+	int err;
+	*accumulator_24 = 0;
+	*sample_count = 0;
+
+	/* 48-bit register read */
+	err = i2c_smbus_read_i2c_block_data(client, reg, 6, data);
+	if (err < 0)
+		return err;
+	if (err != 6)
+		return -EIO;
+	*accumulator_24 = ((data[3] << 16) |
+				(data[4] << 8) |
+				data[5]);
+	*sample_count = ((data[0] << 16) |
+				(data[1] << 8) |
+				data[2]);
+
+	return 0;
+}
+
+static ssize_t sq52205_avg_power_show(struct device *dev,
+					struct device_attribute *da, char *buf)
+{
+	struct sq522xx_data *data = dev_get_drvdata(dev);
+	long sample_count, accumulator_24, regval;
+	int status;
+
+	status = sq52205_read_reg48(data->client, SQ52205_EIN,
+						&accumulator_24, &sample_count);
+	if (status)
+		return status;
+	regval = DIV_ROUND_CLOSEST(accumulator_24, sample_count);
+	regval = regval * data->config->power_lsb;
+
+
+	return sysfs_emit(buf, "%li\n", regval);
+}
+
+/* shunt voltage */
+static SENSOR_DEVICE_ATTR_RO(in0_input, sq522xx_value, SQ522XX_SHUNT_VOLTAGE);
+
+/* bus voltage */
+static SENSOR_DEVICE_ATTR_RO(in1_input, sq522xx_value, SQ522XX_BUS_VOLTAGE);
+
+/* calculated current */
+static SENSOR_DEVICE_ATTR_RO(curr1_input, sq522xx_value, SQ522XX_CURRENT);
+
+/* calculated power */
+static SENSOR_DEVICE_ATTR_RO(power1_input, sq522xx_value, SQ522XX_POWER);
+
+/* shunt resistance */
+static SENSOR_DEVICE_ATTR_RW(shunt_resistor, sq522xx_shunt, SQ522XX_CALIBRATION);
+
+/* update interval (sq52205 only) */
+static SENSOR_DEVICE_ATTR_RW(update_interval, sq52205_interval, 0);
+
+/* calculate_avg_power (sq52205 only) */
+static SENSOR_DEVICE_ATTR_RO(calculate_avg_power, sq52205_avg_power, 0);
+
+
+/* pointers to created device attributes */
+static struct attribute *sq522xx_attrs[] = {
+	&sensor_dev_attr_in0_input.dev_attr.attr,
+	&sensor_dev_attr_in1_input.dev_attr.attr,
+	&sensor_dev_attr_curr1_input.dev_attr.attr,
+	&sensor_dev_attr_power1_input.dev_attr.attr,
+	&sensor_dev_attr_shunt_resistor.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group sq522xx_group = {
+	.attrs = sq522xx_attrs,
+};
+
+static struct attribute *sq52205_attrs[] = {
+	&sensor_dev_attr_update_interval.dev_attr.attr,
+	&sensor_dev_attr_calculate_avg_power.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group sq52205_group = {
+	.attrs = sq52205_attrs,
+};
+
+static const struct i2c_device_id sq522xx_id[];
+
+static int sq522xx_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct sq522xx_data *data;
+	struct device *hwmon_dev;
+	u32 val;
+	int ret, group = 0;
+	enum sq522xx_ids chip;
+
+	if (client->dev.of_node)
+		chip = (uintptr_t)of_device_get_match_data(&client->dev);
+	else
+		chip = i2c_match_id(sq522xx_id, client)->driver_data;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	/* set the device type */
+	data->client = client;
+	data->config = &sq522xx_config[chip];
+	mutex_init(&data->config_lock);
+
+	if (of_property_read_u32(dev->of_node, "shunt-resistor", &val) < 0)
+		val = SQ522XX_RSHUNT_DEFAULT;
+
+
+	if (val <= 0 || val > data->config->calibration_factor)
+		return -ENODEV;
+
+	data->rshunt = val;
+
+	sq522xx_regmap_config.max_register = data->config->registers;
+
+	data->regmap = devm_regmap_init_i2c(client, &sq522xx_regmap_config);
+	if (IS_ERR(data->regmap)) {
+		dev_err(dev, "failed to allocate register map\n");
+		return PTR_ERR(data->regmap);
+	}
+
+
+	ret = sq522xx_init(data);
+	if (ret < 0) {
+		dev_err(dev, "error configuring the device: %d\n", ret);
+		return -ENODEV;
+	}
+	if (chip == sq52205) {
+		ret = sq52205_init(data);
+		if (ret < 0) {
+			dev_err(dev, "error configuring the device cal: %d\n", ret);
+			return -ENODEV;
+		}
+	}
+
+	data->groups[group++] = &sq522xx_group;
+	if (chip == sq52205)
+		data->groups[group++] = &sq52205_group;
+
+	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
+								data, data->groups);
+	if (IS_ERR(hwmon_dev))
+		return PTR_ERR(hwmon_dev);
+
+	dev_info(dev, "power monitor %s (Rshunt = %li uOhm)\n",
+		 client->name, data->rshunt);
+
+	return 0;
+}
+
+static const struct i2c_device_id sq522xx_id[] = {
+	{ "sq52205", sq52205 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, sq522xx_id);
+
+static const struct of_device_id __maybe_unused sq522xx_of_match[] = {
+	{
+		.compatible = "silergy,sq52205",
+		.data = (void *)sq52205
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, sq522xx_of_match);
+
+static struct i2c_driver sq522xx_driver = {
+	.driver = {
+		.name	= "sq522xx",
+		.of_match_table = of_match_ptr(sq522xx_of_match),
+	},
+	.probe		= sq522xx_probe,
+	.id_table	= sq522xx_id,
+};
+
+module_i2c_driver(sq522xx_driver);
+
+MODULE_AUTHOR(" <wenliang202407@163.com> ");
+MODULE_DESCRIPTION("SQ522xx driver");
+MODULE_LICENSE("GPL");
-- 
2.17.1

Re: [PATCH linux dev 6.11] hwmon:add new hwmon driver sq52205

[Guenter Roeck]

On 8/22/24 00:44, Wenliang wrote:
> Thank you for bringing up your questions and suggestions.The SQ52205 is a
> part number specific to the Asian region, which is why you might not be
> able to find it through a search. I'll provide you the website
> (https://us1.silergy.com/zh/productsview/SQ52205FBP).

That page does not point to the chip datasheet. The almost identical
page at https://us1.silergy.com/productsview/SQ52205FBP does.
The datasheet is _not_ "Publicly available" as claimed in this submission.
The version I was able to obtain is tagged with "Silergy Confidential For
<my employer>", so I am not even sure if I can use it to review this driver
submission.

> Some registers of this chip are similar to those of the INA226, but it has
> additional registers such as integrators, which is the main reason why I'm
> offering a new driver.And I plan add drivers of the same series based on

That is not a reason to add a separate driver. Look at, for example, lm90.c,
which supports a variety of chips in a single driver. The ina2xx driver already
does support several chips, and adding another one would be straightforward,
even if it is from a different manufacturer. On top of that, only the EIN and
ACCUM_CONFIG registers are additional, and the rest appear to be exactly the
same as INA226.

> this. I commit the new patch and look forward to your reply.
> 

Additonal comments:
- Please review and follow
   Documentation/process/submitting-patches.rst
   Documentation/hwmon/submitting-patches.rst
- As mentioned before, a reworked version of the ina2xx.c driver is
   available. I am not inclined to accept a new driver for this chip.
   Even if I were, I would not accept a driver based on deprecated
   hwmon APIs. I would strongly advise to have a look into the reworked
   driver. As mentioned before, it is available in the hwmon-staging branch
   of git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git.
- "calculate_avg_power" is (unnecessarily) not a standard attribute and
   therefore unacceptable. Its value (on top of the already averaged power
   attribute) seems questionable. I would understand an attempt to report
   the energy, but I fail to understand the value in reporting yet another
   power average - even more so one that is not well defined in terms of
   number of samples used to determine the average.

Guenter

Re: [PATCH linux dev 6.11] hwmon:add new hwmon driver sq52205

[Guenter Roeck]

On 8/25/24 15:50, Guenter Roeck wrote:
> On 8/22/24 00:44, Wenliang wrote:
>> Thank you for bringing up your questions and suggestions.The SQ52205 is a
>> part number specific to the Asian region, which is why you might not be
>> able to find it through a search. I'll provide you the website
>> (https://us1.silergy.com/zh/productsview/SQ52205FBP).
> 
> That page does not point to the chip datasheet. The almost identical
> page at https://us1.silergy.com/productsview/SQ52205FBP does.

... and that page is no longer available. Given that, if you really need
support for this chip, you might want to consider adding support for
SY24655 and/or SY24657 since public datasheets are available for those
chips. The EIN and ACCUM_CONFIG registers in those chips match the
definitions in your driver submission, so at least at first glance this
should work.

I noticed that there is also SY24656, which appears to be register compatible
to INA226/INA230.

Thanks,
Guenter

[PATCH linux dev-6.11 1/2] hwmon: modified ina2xx to match SY24655(SQ52205)

[Wenliang]

After listening to your advice, I have adapted SQ52205 by rewriting the
ina2xx driver.At the same time, I would like to clarify that SY24655 and
SQ52205 are different partnumber of the same chip. Therefore, you can
refer to SY24655FBP. I have also changed the naming within the driver to
SY24655, and I hope to receive your response.

Signed-off-by: Wenliang <wenliang202407@163.com>
---
 Documentation/hwmon/ina2xx.rst |  24 ++++++++
 drivers/hwmon/Kconfig          |   2 +-
 drivers/hwmon/ina2xx.c         | 106 +++++++++++++++++++++++++++++++--
 3 files changed, 126 insertions(+), 6 deletions(-)

diff --git a/Documentation/hwmon/ina2xx.rst b/Documentation/hwmon/ina2xx.rst
index 27d2e39bc8ac..0bd16a0104a7 100644
--- a/Documentation/hwmon/ina2xx.rst
+++ b/Documentation/hwmon/ina2xx.rst
@@ -53,6 +53,16 @@ Supported chips:
 
 	       https://www.ti.com/
 
+  * Silergy SY24655
+
+
+    Prefix: 'sy24655'
+    Addresses: I2C 0x40 - 0x4f
+
+    Datasheet: Publicly available at the Silergy website
+
+	       https://us1.silergy.com/
+
 Author: Lothar Felten <lothar.felten@gmail.com>
 
 Description
@@ -72,6 +82,11 @@ INA230 and INA231 are high or low side current shunt and power monitors
 with an I2C interface. The chips monitor both a shunt voltage drop and
 bus supply voltage.
 
+The SY24655 is a high- and low-side current shunt and power monitor with an I2C
+interface. The SY24655 both shunt drop and supply voltage, with programmable
+calibration value and conversion times. The SY24655 can also calculate average
+power for use in energy conversion.
+
 The shunt value in micro-ohms can be set via platform data or device tree at
 compile-time or via the shunt_resistor attribute in sysfs at run-time. Please
 refer to the Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml for bindings
@@ -113,6 +128,15 @@ update_interval		data conversion time; affects number of samples used
 			to average results for shunt and bus voltages.
 ======================= ====================================================
 
+Sysfs entries for sy24655 only
+------------------------------------------------
+
+======================= ====================================================
+update_interval		data conversion time; affects number of samples used
+			to average results for shunt and bus voltages.
+calculate_avg_power	calculate average power from last reading to the present.
+======================= ====================================================
+
 .. note::
 
    - Configure `shunt_resistor` before configure `power1_crit`, because power
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index b60fe2e58ad6..1f9752689ae8 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2138,7 +2138,7 @@ config SENSORS_INA2XX
 	select REGMAP_I2C
 	help
 	  If you say yes here you get support for INA219, INA220, INA226,
-	  INA230, and INA231 power monitor chips.
+	  INA230, INA231, and Silergy SY24655 power monitor chips.
 
 	  The INA2xx driver is configured for the default configuration of
 	  the part as described in the datasheet.
diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 9ab4205622e2..34d474ac0b66 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -18,6 +18,10 @@
  * Bi-directional Current/Power Monitor with I2C Interface
  * Datasheet: https://www.ti.com/product/ina230
  *
+ * SY24655:
+ * Bi-directional Current/Power Monitor with I2C Interface
+ * Datasheet: https://us1.silergy.com/productsview/SY24655FBP
+ *
  * Copyright (C) 2012 Lothar Felten <lothar.felten@gmail.com>
  * Thanks to Jan Volkering
  */
@@ -51,15 +55,23 @@
 #define INA226_ALERT_LIMIT		0x07
 #define INA226_DIE_ID			0xFF
 
+/* SY24655 register definitions */
+#define SY24655_EIN				0x0A
+#define SY24655_ACCUM_CONFIG	0x0D
+
 /* register count */
 #define INA219_REGISTERS		6
 #define INA226_REGISTERS		8
+#define SY24655_REGISTERS		0x0D
 
-#define INA2XX_MAX_REGISTERS		8
+#define INA2XX_MAX_REGISTERS		0x0D
 
 /* settings - depend on use case */
 #define INA219_CONFIG_DEFAULT		0x399F	/* PGA=8 */
 #define INA226_CONFIG_DEFAULT		0x4527	/* averages=16 */
+#define SY24655_CONFIG_DEFAULT		0x4527	/* averages=16 */
+/* (only for sy24655) */
+#define SY24655_ACCUM_CONFIG_DEFAULT	0x044C
 
 /* worst case is 68.10 ms (~14.6Hz, ina219) */
 #define INA2XX_CONVERSION_RATE		15
@@ -103,7 +115,7 @@ static struct regmap_config ina2xx_regmap_config = {
 	.val_bits = 16,
 };
 
-enum ina2xx_ids { ina219, ina226 };
+enum ina2xx_ids { ina219, ina226, sy24655};
 
 struct ina2xx_config {
 	u16 config_default;
@@ -117,12 +129,13 @@ struct ina2xx_config {
 
 struct ina2xx_data {
 	const struct ina2xx_config *config;
-
+
 	long rshunt;
 	long current_lsb_uA;
 	long power_lsb_uW;
 	struct mutex config_lock;
 	struct regmap *regmap;
+	struct i2c_client *client;
 
 	const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
 };
@@ -146,6 +159,15 @@ static const struct ina2xx_config ina2xx_config[] = {
 		.bus_voltage_lsb = 1250,
 		.power_lsb_factor = 25,
 	},
+	[sy24655] = {
+		.config_default = SY24655_CONFIG_DEFAULT,
+		.calibration_value = 2048,
+		.registers = SY24655_REGISTERS,
+		.shunt_div = 400,
+		.bus_voltage_shift = 0,
+		.bus_voltage_lsb = 1250,
+		.power_lsb_factor = 25,
+	},
 };
 
 /*
@@ -216,6 +238,12 @@ static int ina2xx_init(struct ina2xx_data *data)
 	return ina2xx_calibrate(data);
 }
 
+static int sy24655_init(struct ina2xx_data *data)
+{
+	return regmap_write(data->regmap, SY24655_ACCUM_CONFIG,
+				SY24655_ACCUM_CONFIG_DEFAULT);
+}
+
 static int ina2xx_read_reg(struct device *dev, int reg, unsigned int *regval)
 {
 	struct ina2xx_data *data = dev_get_drvdata(dev);
@@ -551,6 +579,48 @@ static ssize_t ina226_interval_show(struct device *dev,
 	return sysfs_emit(buf, "%d\n", ina226_reg_to_interval(regval));
 }
 
+static int sy24655_read_reg48(const struct i2c_client *client, u8 reg,
+					long *accumulator_24, long *sample_count)
+{
+	u8 data[6];
+	int err;
+	*accumulator_24 = 0;
+	*sample_count = 0;
+
+	/* 48-bit register read */
+	err = i2c_smbus_read_i2c_block_data(client, reg, 6, data);
+	if (err < 0)
+		return err;
+	if (err != 6)
+		return -EIO;
+	*accumulator_24 = ((data[3] << 16) |
+				(data[4] << 8) |
+				data[5]);
+	*sample_count = ((data[0] << 16) |
+				(data[1] << 8) |
+				data[2]);
+
+	return 0;
+}
+
+static ssize_t sy24655_avg_power_show(struct device *dev,
+					struct device_attribute *da, char *buf)
+{
+	struct ina2xx_data *data = dev_get_drvdata(dev);
+	long sample_count, accumulator_24, regval;
+	int status;
+
+	status = sy24655_read_reg48(data->client, SY24655_EIN,
+						&accumulator_24, &sample_count);
+	if (status)
+		return status;
+	regval = DIV_ROUND_CLOSEST(accumulator_24, sample_count);
+	regval = regval * data->power_lsb_uW;
+
+
+	return sysfs_emit(buf, "%li\n", regval);
+}
+
 /* shunt voltage */
 static SENSOR_DEVICE_ATTR_RO(in0_input, ina2xx_value, INA2XX_SHUNT_VOLTAGE);
 /* shunt voltage over/under voltage alert setting and alarm */
@@ -589,9 +659,13 @@ static SENSOR_DEVICE_ATTR_RO(power1_crit_alarm, ina226_alarm,
 /* shunt resistance */
 static SENSOR_DEVICE_ATTR_RW(shunt_resistor, ina2xx_shunt, INA2XX_CALIBRATION);
 
-/* update interval (ina226 only) */
+/* update interval (ina226 and sy24655) */
 static SENSOR_DEVICE_ATTR_RW(update_interval, ina226_interval, 0);
 
+/* calculate_avg_power (sy24655 only) */
+static SENSOR_DEVICE_ATTR_RO(calculate_avg_power, sy24655_avg_power, 0);
+
+
 /* pointers to created device attributes */
 static struct attribute *ina2xx_attrs[] = {
 	&sensor_dev_attr_in0_input.dev_attr.attr,
@@ -624,6 +698,15 @@ static struct attribute *ina226_attrs[] = {
 static const struct attribute_group ina226_group = {
 	.attrs = ina226_attrs,
 };
+static struct attribute *sy24655_attrs[] = {
+	&sensor_dev_attr_update_interval.dev_attr.attr,
+	&sensor_dev_attr_calculate_avg_power.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group sy24655_group = {
+	.attrs = sy24655_attrs,
+};
 
 static int ina2xx_probe(struct i2c_client *client)
 {
@@ -641,6 +724,7 @@ static int ina2xx_probe(struct i2c_client *client)
 		return -ENOMEM;
 
 	/* set the device type */
+	data->client = client;
 	data->config = &ina2xx_config[chip];
 	mutex_init(&data->config_lock);
 
@@ -691,10 +775,17 @@ static int ina2xx_probe(struct i2c_client *client)
 		dev_err(dev, "error configuring the device: %d\n", ret);
 		return -ENODEV;
 	}
-
+	if (chip == sy24655)
+		ret = sy24655_init(data);
+	if (ret < 0) {
+		dev_err(dev, "error configuring the accum_reg: %d\n", ret);
+		return -ENODEV;
+	}
 	data->groups[group++] = &ina2xx_group;
 	if (chip == ina226)
 		data->groups[group++] = &ina226_group;
+	else if (chip == sy24655)
+		data->groups[group++] = &sy24655_group;
 
 	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
 							   data, data->groups);
@@ -713,6 +804,7 @@ static const struct i2c_device_id ina2xx_id[] = {
 	{ "ina226", ina226 },
 	{ "ina230", ina226 },
 	{ "ina231", ina226 },
+	{ "sy24655", sy24655},
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, ina2xx_id);
@@ -738,6 +830,10 @@ static const struct of_device_id __maybe_unused ina2xx_of_match[] = {
 		.compatible = "ti,ina231",
 		.data = (void *)ina226
 	},
+	{
+		.compatible = "silergy,sy24655",
+		.data = (void *)sy24655
+	},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, ina2xx_of_match);
-- 
2.17.1

[PATCH linux dev-6.11 2/2] dt-bindings: modified ina2xx to match SY24655(SQ52205)

[Wenliang]

Modified the binding of ina2xx to make it compatible with SY24655. 

Signed-off-by: Wenliang <wenliang202407@163.com>
---
 Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
index 6ae961732e6b..400e7cefad17 100644
--- a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
+++ b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
@@ -29,6 +29,7 @@ properties:
       - ti,ina237
       - ti,ina238
       - ti,ina260
+      - silergy,sy24655
 
   reg:
     maxItems: 1
-- 
2.17.1

Re: [PATCH linux dev-6.11 2/2] dt-bindings: modified ina2xx to match SY24655(SQ52205)

[Conor Dooley]

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

On Wed, Sep 11, 2024 at 08:25:18AM -0400, Wenliang wrote:
> Modified the binding of ina2xx to make it compatible with SY24655. 

Rather, you should explain why the sy24655 is compatible with the ina2xx
devices.

> 
> Signed-off-by: Wenliang <wenliang202407@163.com>
> ---
>  Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
> index 6ae961732e6b..400e7cefad17 100644
> --- a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
> @@ -29,6 +29,7 @@ properties:
>        - ti,ina237
>        - ti,ina238
>        - ti,ina260
> +      - silergy,sy24655

Please add this in alphabetical order.

Thanks,
Conor.

>  
>    reg:
>      maxItems: 1
> -- 
> 2.17.1
> 

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

Re: [PATCH linux dev-6.11 2/2] dt-bindings: modified ina2xx to match SY24655(SQ52205)

[Krzysztof Kozlowski]

On 11/09/2024 19:49, Conor Dooley wrote:
> On Wed, Sep 11, 2024 at 08:25:18AM -0400, Wenliang wrote:
>> Modified the binding of ina2xx to make it compatible with SY24655. 
> 
> Rather, you should explain why the sy24655 is compatible with the ina2xx
> devices.
> 
>>

This looks like patch for some forked tree, like the BMC folks are
doing. At least linux dev suggests it.

Best regards,
Krzysztof

Re: [PATCH linux dev-6.11 2/2] dt-bindings: modified ina2xx to match SY24655(SQ52205)

[Conor Dooley]

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

On Tue, Oct 08, 2024 at 02:35:38PM +0200, Krzysztof Kozlowski wrote:
> On 11/09/2024 19:49, Conor Dooley wrote:
> > On Wed, Sep 11, 2024 at 08:25:18AM -0400, Wenliang wrote:
> >> Modified the binding of ina2xx to make it compatible with SY24655. 
> > 
> > Rather, you should explain why the sy24655 is compatible with the ina2xx
> > devices.
> > 
> >>
> 
> This looks like patch for some forked tree, like the BMC folks are
> doing. At least linux dev suggests it.

CC list wouldn't imply that it is.

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

Re: Re: [PATCH linux dev-6.11 2/2] dt-bindings: modified ina2xx to match SY24655(SQ52205)

[Conor Dooley]

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

Hey,

On Tue, Oct 08, 2024 at 07:58:51PM +0800, wenliang yan wrote:
> Modified the binding of ina2xx to make it compatible with SY24655. 
> 
> 
> 
> 
> Signed-off-by: Wenliang <wenliang202407@163.com>
> 
> ---
> 
> 
> 
> 
> SY24655 is a fixed gain power monitor from Silergy, with a power supply
> 
> of 2.7-5.5V and communication mode of IIC capable of detecting bus voltage
> 
> and voltage on shunt resistors. Its first 5 registers are identical to
> 
> ina226, and also have alert and limit functions. So, the sy24655 is
> 
> compatible with the ina2xx devices.

This should be above the signoff and --- line. Your patch is pretty
badly malformed, did you use b4 or git send-email to submit it?

Cheers,
Conor.

> 
> 
> 
> 
>  Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml | 1 +
> 
>  1 file changed, 1 insertion(+)
> 
> 
> 
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
> 
> index 6ae961732e6b..05a9cb36cd82 100644
> 
> --- a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
> 
> +++ b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
> 
> @@ -20,6 +20,7 @@ description: |
> 
>  properties:
> 
>    compatible:
> 
>      enum:
> 
> +      - silergy,sy24655
> 
>        - ti,ina209
> 
>        - ti,ina219
> 
>        - ti,ina220
> 
> -- 
> 
> 2.17.1
> 
> 

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

Re: [PATCH linux dev-6.11 1/2] hwmon: modified ina2xx to match SY24655(SQ52205)

[Guenter Roeck]

On 9/11/24 05:25, Wenliang wrote:
> After listening to your advice, I have adapted SQ52205 by rewriting the
> ina2xx driver.At the same time, I would like to clarify that SY24655 and
> SQ52205 are different partnumber of the same chip. Therefore, you can
> refer to SY24655FBP. I have also changed the naming within the driver to
> SY24655, and I hope to receive your response.
> 

This is not an appropriate patch description. The information is useful,
but should be after '---'. Also, this is the second version of your
patch and should be versioned.

Please read and follow Documentation/process/submitting-patches.rst

You did not use the updated version of the ina2xx driver for this patch.
The updated version is now available in linux-next and will be sent upstream
in the next commit window. Consequently your patch is outdated.

Additional comments inline.

> Signed-off-by: Wenliang <wenliang202407@163.com>
> ---
>   Documentation/hwmon/ina2xx.rst |  24 ++++++++
>   drivers/hwmon/Kconfig          |   2 +-
>   drivers/hwmon/ina2xx.c         | 106 +++++++++++++++++++++++++++++++--
>   3 files changed, 126 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/hwmon/ina2xx.rst b/Documentation/hwmon/ina2xx.rst
> index 27d2e39bc8ac..0bd16a0104a7 100644
> --- a/Documentation/hwmon/ina2xx.rst
> +++ b/Documentation/hwmon/ina2xx.rst
> @@ -53,6 +53,16 @@ Supported chips:
>   
>   	       https://www.ti.com/
>   
> +  * Silergy SY24655
> +
> +
> +    Prefix: 'sy24655'
> +    Addresses: I2C 0x40 - 0x4f
> +
> +    Datasheet: Publicly available at the Silergy website
> +
> +	       https://us1.silergy.com/
> +
>   Author: Lothar Felten <lothar.felten@gmail.com>
>   
>   Description
> @@ -72,6 +82,11 @@ INA230 and INA231 are high or low side current shunt and power monitors
>   with an I2C interface. The chips monitor both a shunt voltage drop and
>   bus supply voltage.
>   
> +The SY24655 is a high- and low-side current shunt and power monitor with an I2C
> +interface. The SY24655 both shunt drop and supply voltage, with programmable
> +calibration value and conversion times. The SY24655 can also calculate average
> +power for use in energy conversion.
> +
>   The shunt value in micro-ohms can be set via platform data or device tree at
>   compile-time or via the shunt_resistor attribute in sysfs at run-time. Please
>   refer to the Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml for bindings
> @@ -113,6 +128,15 @@ update_interval		data conversion time; affects number of samples used
>   			to average results for shunt and bus voltages.
>   ======================= ====================================================
>   
> +Sysfs entries for sy24655 only
> +------------------------------------------------
> +
> +======================= ====================================================
> +update_interval		data conversion time; affects number of samples used
> +			to average results for shunt and bus voltages.

The above is not for sy24655 only; it also applies to ina226 and compatible chips.

> +calculate_avg_power	calculate average power from last reading to the present.

Why not the standard power1_average ? I don't see a reason to introduce a non-standard
attribute.

> +======================= ====================================================
> +
>   .. note::
>   
>      - Configure `shunt_resistor` before configure `power1_crit`, because power
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index b60fe2e58ad6..1f9752689ae8 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2138,7 +2138,7 @@ config SENSORS_INA2XX
>   	select REGMAP_I2C
>   	help
>   	  If you say yes here you get support for INA219, INA220, INA226,
> -	  INA230, and INA231 power monitor chips.
> +	  INA230, INA231, and Silergy SY24655 power monitor chips.
>   
>   	  The INA2xx driver is configured for the default configuration of
>   	  the part as described in the datasheet.
> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index 9ab4205622e2..34d474ac0b66 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -18,6 +18,10 @@
>    * Bi-directional Current/Power Monitor with I2C Interface
>    * Datasheet: https://www.ti.com/product/ina230
>    *
> + * SY24655:
> + * Bi-directional Current/Power Monitor with I2C Interface
> + * Datasheet: https://us1.silergy.com/productsview/SY24655FBP
> + *
>    * Copyright (C) 2012 Lothar Felten <lothar.felten@gmail.com>
>    * Thanks to Jan Volkering
>    */
> @@ -51,15 +55,23 @@
>   #define INA226_ALERT_LIMIT		0x07
>   #define INA226_DIE_ID			0xFF
>   
> +/* SY24655 register definitions */
> +#define SY24655_EIN				0x0A
> +#define SY24655_ACCUM_CONFIG	0x0D
> +
>   /* register count */
>   #define INA219_REGISTERS		6
>   #define INA226_REGISTERS		8
> +#define SY24655_REGISTERS		0x0D
>   
> -#define INA2XX_MAX_REGISTERS		8
> +#define INA2XX_MAX_REGISTERS		0x0D
>   
>   /* settings - depend on use case */
>   #define INA219_CONFIG_DEFAULT		0x399F	/* PGA=8 */
>   #define INA226_CONFIG_DEFAULT		0x4527	/* averages=16 */
> +#define SY24655_CONFIG_DEFAULT		0x4527	/* averages=16 */
> +/* (only for sy24655) */
> +#define SY24655_ACCUM_CONFIG_DEFAULT	0x044C
>   
>   /* worst case is 68.10 ms (~14.6Hz, ina219) */
>   #define INA2XX_CONVERSION_RATE		15
> @@ -103,7 +115,7 @@ static struct regmap_config ina2xx_regmap_config = {
>   	.val_bits = 16,
>   };
>   
> -enum ina2xx_ids { ina219, ina226 };
> +enum ina2xx_ids { ina219, ina226, sy24655};
>   
>   struct ina2xx_config {
>   	u16 config_default;
> @@ -117,12 +129,13 @@ struct ina2xx_config {
>   
>   struct ina2xx_data {
>   	const struct ina2xx_config *config;
> -
> +
>   	long rshunt;
>   	long current_lsb_uA;
>   	long power_lsb_uW;
>   	struct mutex config_lock;
>   	struct regmap *regmap;
> +	struct i2c_client *client;
>   
>   	const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
>   };
> @@ -146,6 +159,15 @@ static const struct ina2xx_config ina2xx_config[] = {
>   		.bus_voltage_lsb = 1250,
>   		.power_lsb_factor = 25,
>   	},
> +	[sy24655] = {
> +		.config_default = SY24655_CONFIG_DEFAULT,
> +		.calibration_value = 2048,
> +		.registers = SY24655_REGISTERS,
> +		.shunt_div = 400,
> +		.bus_voltage_shift = 0,
> +		.bus_voltage_lsb = 1250,
> +		.power_lsb_factor = 25,
> +	},
>   };
>   
>   /*
> @@ -216,6 +238,12 @@ static int ina2xx_init(struct ina2xx_data *data)
>   	return ina2xx_calibrate(data);
>   }
>   
> +static int sy24655_init(struct ina2xx_data *data)
> +{
> +	return regmap_write(data->regmap, SY24655_ACCUM_CONFIG,
> +				SY24655_ACCUM_CONFIG_DEFAULT);
> +}
> +
>   static int ina2xx_read_reg(struct device *dev, int reg, unsigned int *regval)
>   {
>   	struct ina2xx_data *data = dev_get_drvdata(dev);
> @@ -551,6 +579,48 @@ static ssize_t ina226_interval_show(struct device *dev,
>   	return sysfs_emit(buf, "%d\n", ina226_reg_to_interval(regval));
>   }
>   
> +static int sy24655_read_reg48(const struct i2c_client *client, u8 reg,
> +					long *accumulator_24, long *sample_count)
> +{
> +	u8 data[6];
> +	int err;
> +	*accumulator_24 = 0;
> +	*sample_count = 0;
> +
> +	/* 48-bit register read */
> +	err = i2c_smbus_read_i2c_block_data(client, reg, 6, data);
> +	if (err < 0)
> +		return err;
> +	if (err != 6)
> +		return -EIO;
> +	*accumulator_24 = ((data[3] << 16) |
> +				(data[4] << 8) |
> +				data[5]);
> +	*sample_count = ((data[0] << 16) |
> +				(data[1] << 8) |
> +				data[2]);
> +
> +	return 0;
> +}
> +
> +static ssize_t sy24655_avg_power_show(struct device *dev,
> +					struct device_attribute *da, char *buf)
> +{
> +	struct ina2xx_data *data = dev_get_drvdata(dev);
> +	long sample_count, accumulator_24, regval;
> +	int status;
> +
> +	status = sy24655_read_reg48(data->client, SY24655_EIN,
> +						&accumulator_24, &sample_count);
> +	if (status)
> +		return status;
> +	regval = DIV_ROUND_CLOSEST(accumulator_24, sample_count);

Since the sample count is not used anywhere else, it would make sense
to just read and return the average power in a single function. Also
make sure that sample_count isn't 0.

Note that this does not return the "average power from last reading
to the present" as claimed above unless bit 1 of ACCUM_CONFIG is set
to 0. Actually I am not sure what exactly it reports since the sample
count and the accumulator values will overflow at different times.
As far as I can see it returns a more or less random value after an
overflow.

Are you sure this provides any value as implemented ? I really don't see
what that value would be, especially since there is no means to account
for overflows.

> +	regval = regval * data->power_lsb_uW;
> +
> +
> +	return sysfs_emit(buf, "%li\n", regval);
> +}
> +
>   /* shunt voltage */
>   static SENSOR_DEVICE_ATTR_RO(in0_input, ina2xx_value, INA2XX_SHUNT_VOLTAGE);
>   /* shunt voltage over/under voltage alert setting and alarm */
> @@ -589,9 +659,13 @@ static SENSOR_DEVICE_ATTR_RO(power1_crit_alarm, ina226_alarm,
>   /* shunt resistance */
>   static SENSOR_DEVICE_ATTR_RW(shunt_resistor, ina2xx_shunt, INA2XX_CALIBRATION);
>   
> -/* update interval (ina226 only) */
> +/* update interval (ina226 and sy24655) */
>   static SENSOR_DEVICE_ATTR_RW(update_interval, ina226_interval, 0);
>   
> +/* calculate_avg_power (sy24655 only) */
> +static SENSOR_DEVICE_ATTR_RO(calculate_avg_power, sy24655_avg_power, 0);
> +
> +
>   /* pointers to created device attributes */
>   static struct attribute *ina2xx_attrs[] = {
>   	&sensor_dev_attr_in0_input.dev_attr.attr,
> @@ -624,6 +698,15 @@ static struct attribute *ina226_attrs[] = {
>   static const struct attribute_group ina226_group = {
>   	.attrs = ina226_attrs,
>   };
> +static struct attribute *sy24655_attrs[] = {
> +	&sensor_dev_attr_update_interval.dev_attr.attr,
> +	&sensor_dev_attr_calculate_avg_power.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group sy24655_group = {
> +	.attrs = sy24655_attrs,
> +};
>   
>   static int ina2xx_probe(struct i2c_client *client)
>   {
> @@ -641,6 +724,7 @@ static int ina2xx_probe(struct i2c_client *client)
>   		return -ENOMEM;
>   
>   	/* set the device type */
> +	data->client = client;
>   	data->config = &ina2xx_config[chip];
>   	mutex_init(&data->config_lock);
>   
> @@ -691,10 +775,17 @@ static int ina2xx_probe(struct i2c_client *client)
>   		dev_err(dev, "error configuring the device: %d\n", ret);
>   		return -ENODEV;
>   	}
> -
> +	if (chip == sy24655)
> +		ret = sy24655_init(data);
> +	if (ret < 0) {
> +		dev_err(dev, "error configuring the accum_reg: %d\n", ret);
> +		return -ENODEV;
> +	}
>   	data->groups[group++] = &ina2xx_group;
>   	if (chip == ina226)
>   		data->groups[group++] = &ina226_group;
> +	else if (chip == sy24655)
> +		data->groups[group++] = &sy24655_group;
>   
>   	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
>   							   data, data->groups);
> @@ -713,6 +804,7 @@ static const struct i2c_device_id ina2xx_id[] = {
>   	{ "ina226", ina226 },
>   	{ "ina230", ina226 },
>   	{ "ina231", ina226 },
> +	{ "sy24655", sy24655},
>   	{ }
>   };
>   MODULE_DEVICE_TABLE(i2c, ina2xx_id);
> @@ -738,6 +830,10 @@ static const struct of_device_id __maybe_unused ina2xx_of_match[] = {
>   		.compatible = "ti,ina231",
>   		.data = (void *)ina226
>   	},
> +	{
> +		.compatible = "silergy,sy24655",
> +		.data = (void *)sy24655
> +	},
>   	{ },
>   };
>   MODULE_DEVICE_TABLE(of, ina2xx_of_match);

[PATCH linux dev-6.11 1/2] v3: hwmon: modified ina2xx to match SY24655

[Wenliang]

This change makes the INA2XX driver compatible with sy24655,
involving the reading of average power

Signed-off-by: Wenliang <wenliang202407@163.com>
---

This change uses the latest ina2xx driver under the Linux NEXT branch.
I will explain the operation and significance of the average power
read function. I configure the READ_EIN (bit 10) of the ACCUM_CONFIG
register to 1, so that every time the EIN register is read, it will be
cleared. Therefore, the average power from the current read to the
previous read can be directly calculated. The reason for adopting 
this continuous reading mode is to facilitate upper level users to
use more accurate time to convert energy instead of using internal
time intervals.

 Documentation/hwmon/ina2xx.rst |  25 +++++++-
 drivers/hwmon/Kconfig          |   2 +-
 drivers/hwmon/ina2xx.c         | 103 +++++++++++++++++++++++++++++++--
 3 files changed, 124 insertions(+), 6 deletions(-)

diff --git a/Documentation/hwmon/ina2xx.rst b/Documentation/hwmon/ina2xx.rst
index 1ce161e6c0bf..eac8bb1deda0 100644
--- a/Documentation/hwmon/ina2xx.rst
+++ b/Documentation/hwmon/ina2xx.rst
@@ -63,6 +63,16 @@ Supported chips:
 
 	       https://www.ti.com/
 
+  * Silergy SY24655
+
+
+    Prefix: 'sy24655'
+    Addresses: I2C 0x40 - 0x4f
+
+    Datasheet: Publicly available at the Silergy website
+
+	       https://us1.silergy.com/
+
 Author: Lothar Felten <lothar.felten@gmail.com>
 
 Description
@@ -85,6 +95,11 @@ bus supply voltage.
 INA260 is a high or low side current and power monitor with integrated shunt
 resistor.
 
+The SY24655 is a high- and low-side current shunt and power monitor with an I2C
+interface. The SY24655 both shunt drop and supply voltage, with programmable
+calibration value and conversion times. The SY24655 can also calculate average
+power for use in energy conversion.
+
 The shunt value in micro-ohms can be set via platform data or device tree at
 compile-time or via the shunt_resistor attribute in sysfs at run-time. Please
 refer to the Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml for bindings
@@ -108,7 +123,7 @@ power1_input		Power(uW) measurement channel
 shunt_resistor		Shunt resistance(uOhm) channel (not for ina260)
 ======================= ===============================================
 
-Additional sysfs entries for ina226, ina230, ina231, and ina260
+Additional sysfs entries for ina226, ina230, ina231, ina260, and sy24655
 ---------------------------------------------------------------
 
 ======================= ====================================================
@@ -130,6 +145,14 @@ update_interval		data conversion time; affects number of samples used
 			to average results for shunt and bus voltages.
 ======================= ====================================================
 
+Sysfs entries for sy24655 only
+------------------------------------------------
+
+======================= ====================================================
+power1_average		calculate average power from last reading to the
+			present.
+======================= ====================================================
+
 .. note::
 
    - Configure `shunt_resistor` before configure `power1_crit`, because power
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 6a3385598cc6..11eb1c5aa358 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2167,7 +2167,7 @@ config SENSORS_INA2XX
 	select REGMAP_I2C
 	help
 	  If you say yes here you get support for INA219, INA220, INA226,
-	  INA230, INA231, and INA260 power monitor chips.
+	  INA230, INA231, INA260, and SY24655 power monitor chips.
 
 	  The INA2xx driver is configured for the default configuration of
 	  the part as described in the datasheet.
diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index cecc80a41a97..89d2d2ae7f15 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -51,12 +51,25 @@
 #define INA226_ALERT_LIMIT		0x07
 #define INA226_DIE_ID			0xFF
 
-#define INA2XX_MAX_REGISTERS		8
+/* SY24655 register definitions */
+#define SY24655_EIN				0x0A
+#define SY24655_ACCUM_CONFIG	0x0D
+
+/* register count */
+#define INA219_REGISTERS		6
+#define INA226_REGISTERS		8
+#define SY24655_REGISTERS		0x0D
+
+#define INA2XX_MAX_REGISTERS		0x0D
 
 /* settings - depend on use case */
 #define INA219_CONFIG_DEFAULT		0x399F	/* PGA=8 */
 #define INA226_CONFIG_DEFAULT		0x4527	/* averages=16 */
 #define INA260_CONFIG_DEFAULT		0x6527	/* averages=16 */
+#define SY24655_CONFIG_DEFAULT		0x4527	/* averages=16 */
+
+/* (only for sy24655) */
+#define SY24655_ACCUM_CONFIG_DEFAULT	0x044C	/* continuous mode, clear after read*/
 
 /* worst case is 68.10 ms (~14.6Hz, ina219) */
 #define INA2XX_CONVERSION_RATE		15
@@ -84,6 +97,8 @@
 #define INA226_ALERT_CONFIG_MASK	GENMASK(15, 10)
 #define INA226_ALERT_FUNCTION_FLAG	BIT(4)
 
+#define SY24655_EIN_OVERFLOW_FLAG	BIT(6)
+
 /*
  * Both bus voltage and shunt voltage conversion times for ina226 are set
  * to 0b0100 on POR, which translates to 2200 microseconds in total.
@@ -97,6 +112,7 @@ static bool ina2xx_writeable_reg(struct device *dev, unsigned int reg)
 	case INA2XX_CALIBRATION:
 	case INA226_MASK_ENABLE:
 	case INA226_ALERT_LIMIT:
+	case SY24655_ACCUM_CONFIG:
 		return true;
 	default:
 		return false;
@@ -110,6 +126,7 @@ static bool ina2xx_volatile_reg(struct device *dev, unsigned int reg)
 	case INA2XX_BUS_VOLTAGE:
 	case INA2XX_POWER:
 	case INA2XX_CURRENT:
+	case SY24655_EIN:
 		return true;
 	default:
 		return false;
@@ -127,7 +144,7 @@ static const struct regmap_config ina2xx_regmap_config = {
 	.writeable_reg = ina2xx_writeable_reg,
 };
 
-enum ina2xx_ids { ina219, ina226, ina260 };
+enum ina2xx_ids { ina219, ina226, ina260, sy24655 };
 
 struct ina2xx_config {
 	u16 config_default;
@@ -149,6 +166,8 @@ struct ina2xx_data {
 	long power_lsb_uW;
 	struct mutex config_lock;
 	struct regmap *regmap;
+	struct i2c_client *client;
+
 };
 
 static const struct ina2xx_config ina2xx_config[] = {
@@ -181,6 +200,16 @@ static const struct ina2xx_config ina2xx_config[] = {
 		.has_alerts = true,
 		.has_ishunt = true,
 	},
+	[sy24655] = {
+		.config_default = SY24655_CONFIG_DEFAULT,
+		.calibration_value = 2048,
+		.shunt_div = 400,
+		.bus_voltage_shift = 0,
+		.bus_voltage_lsb = 1250,
+		.power_lsb_factor = 25,
+		.has_alerts = false,
+		.has_ishunt = false,
+	},
 };
 
 /*
@@ -485,6 +514,49 @@ static int ina2xx_in_read(struct device *dev, u32 attr, int channel, long *val)
 	return 0;
 }
 
+/*
+ * Configuring the READ_EIN (bit 10) of the ACCUM_CONFIG register to 1
+ * can clear accumulator and sample_count after reading the EIN register.
+ * This way, the average power between the last read and the current
+ * read can be obtained. By combining with accurate time data from
+ * outside, the energy consumption during that period can be calculated.
+ */
+static int sy24655_average_power_read(struct ina2xx_data *data, u8 reg, long *val)
+{
+	u8 template[6];
+	int ret;
+	long accumulator_24, sample_count;
+	unsigned int regval;
+
+	ret = regmap_read_bypassed(data->regmap, INA226_MASK_ENABLE, &regval);
+	if (ret)
+		return ret;
+
+	if (regval & SY24655_EIN_OVERFLOW_FLAG)
+		return -ENOMEM;
+
+	/* 48-bit register read */
+	ret = i2c_smbus_read_i2c_block_data(data->client, reg, 6, template);
+	if (ret < 0)
+		return ret;
+	if (ret != 6)
+		return -EIO;
+	accumulator_24 = ((template[3] << 16) |
+				(template[4] << 8) |
+				template[5]);
+	sample_count = ((template[0] << 16) |
+				(template[1] << 8) |
+				template[2]);
+	if (sample_count <= 0) {
+		*val = 0;
+		return 0;
+	}
+
+	*val = DIV_ROUND_CLOSEST(accumulator_24, sample_count) * data->power_lsb_uW;
+
+	return 0;
+}
+
 static int ina2xx_power_read(struct device *dev, u32 attr, long *val)
 {
 	struct ina2xx_data *data = dev_get_drvdata(dev);
@@ -492,6 +564,8 @@ static int ina2xx_power_read(struct device *dev, u32 attr, long *val)
 	switch (attr) {
 	case hwmon_power_input:
 		return ina2xx_read_init(dev, INA2XX_POWER, val);
+	case hwmon_power_average:
+		return sy24655_average_power_read(data, SY24655_EIN, val);
 	case hwmon_power_crit:
 		return ina226_alert_limit_read(data, INA226_POWER_OVER_LIMIT_MASK,
 					       INA2XX_POWER, val);
@@ -702,6 +776,8 @@ static umode_t ina2xx_is_visible(const void *_data, enum hwmon_sensor_types type
 			if (has_alerts)
 				return 0444;
 			break;
+		case hwmon_power_average:
+			return 0444;
 		default:
 			break;
 		}
@@ -734,7 +810,8 @@ static const struct hwmon_channel_info * const ina2xx_info[] = {
 	HWMON_CHANNEL_INFO(curr, HWMON_C_INPUT | HWMON_C_CRIT | HWMON_C_CRIT_ALARM |
 			   HWMON_C_LCRIT | HWMON_C_LCRIT_ALARM),
 	HWMON_CHANNEL_INFO(power,
-			   HWMON_P_INPUT | HWMON_P_CRIT | HWMON_P_CRIT_ALARM),
+			   HWMON_P_INPUT | HWMON_P_CRIT | HWMON_P_CRIT_ALARM |
+			   HWMON_P_AVERAGE),
 	NULL
 };
 
@@ -840,6 +917,18 @@ static int ina2xx_init(struct device *dev, struct ina2xx_data *data)
 						FIELD_PREP(INA226_ALERT_POLARITY, active_high));
 	}
 
+	if (data->chip == sy24655) {
+		/*
+		 * Initialize the power accumulation method to continuous
+		 * mode and clear the EIN register after each read of the
+		 * EIN register
+		 */
+		ret = regmap_write(regmap, SY24655_ACCUM_CONFIG,
+				SY24655_ACCUM_CONFIG_DEFAULT);
+		if (ret < 0)
+			return ret;
+	}
+
 	if (data->config->has_ishunt)
 		return 0;
 
@@ -868,6 +957,7 @@ static int ina2xx_probe(struct i2c_client *client)
 		return -ENOMEM;
 
 	/* set the device type */
+	data->client = client;
 	data->config = &ina2xx_config[chip];
 	data->chip = chip;
 	mutex_init(&data->config_lock);
@@ -906,6 +996,7 @@ static const struct i2c_device_id ina2xx_id[] = {
 	{ "ina230", ina226 },
 	{ "ina231", ina226 },
 	{ "ina260", ina260 },
+	{ "sy24655", sy24655 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, ina2xx_id);
@@ -935,7 +1026,11 @@ static const struct of_device_id __maybe_unused ina2xx_of_match[] = {
 		.compatible = "ti,ina260",
 		.data = (void *)ina260
 	},
-	{ },
+	{
+		.compatible = "silergy,sy24655",
+		.data = (void *)sy24655
+	},
+	{ }
 };
 MODULE_DEVICE_TABLE(of, ina2xx_of_match);
 
-- 
2.17.1

[PATCH linux dev-6.11 2/2] v3: dt-bindings: modified ina2xx to match SY24655

[Wenliang]

Modified the binding of ina2xx to make it compatible with SY24655. 

Signed-off-by: Wenliang <wenliang202407@163.com>
---

SY24655 is a fixed gain power monitor from Silergy, with a power supply
of 2.7-5.5V and communication mode of IIC capable of detecting bus voltage
and voltage on shunt resistors. Its first 5 registers are identical to
ina226, and also have alert and limit functions. So, the sy24655 is
compatible with the ina2xx devices.

 Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
index 6ae961732e6b..05a9cb36cd82 100644
--- a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
+++ b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
@@ -20,6 +20,7 @@ description: |
 properties:
   compatible:
     enum:
+      - silergy,sy24655
       - ti,ina209
       - ti,ina219
       - ti,ina220
-- 
2.17.1

Re: [PATCH linux dev-6.11 2/2] v3: dt-bindings: modified ina2xx to match SY24655

[Krzysztof Kozlowski]

On 12/10/2024 09:06, Wenliang wrote:
> Modified the binding of ina2xx to make it compatible with SY24655. 
> 

Your subject is odd. We do not develop v6.11, that's something already
old. The cc-list suggest you just want it for next release, so rebase on
mainline or next and avoid any unusual patch prefixes.

OTOH, v3 is not part of subject. Use b4 or git format-patch -v3.

> Signed-off-by: Wenliang <wenliang202407@163.com>
> ---
> 
> SY24655 is a fixed gain power monitor from Silergy, with a power supply
> of 2.7-5.5V and communication mode of IIC capable of detecting bus voltage
> and voltage on shunt resistors. Its first 5 registers are identical to
> ina226, and also have alert and limit functions. So, the sy24655 is
> compatible with the ina2xx devices.

This is hardware description, why it is not part of commit message?


Best regards,
Krzysztof

Re: [PATCH linux dev-6.11 2/2] v3: dt-bindings: modified ina2xx to match SY24655

[Guenter Roeck]

On 10/12/24 03:06, Krzysztof Kozlowski wrote:
> On 12/10/2024 09:06, Wenliang wrote:
>> Modified the binding of ina2xx to make it compatible with SY24655.
>>
> 
> Your subject is odd. We do not develop v6.11, that's something already
> old. The cc-list suggest you just want it for next release, so rebase on
> mainline or next and avoid any unusual patch prefixes.
> 

The submitter also still refuses to use imperative mood. I don't know
how often this has to be repeated. I take this as "does not address feedback"
and will not even look at the patches anymore until the reported problems
have been fixed.

Oh, and I just noticed that this version has been sent as response to the
previous version, which is also discouraged. On top of that, there is
no change log.

Guenter

Re: [PATCH linux dev-6.11 1/2] hwmon: modified ina2xx to match SY24655(SQ52205)

[Christophe JAILLET]

Le 11/09/2024 à 14:25, Wenliang a écrit :
> After listening to your advice, I have adapted SQ52205 by rewriting the
> ina2xx driver.At the same time, I would like to clarify that SY24655 and
> SQ52205 are different partnumber of the same chip. Therefore, you can
> refer to SY24655FBP. I have also changed the naming within the driver to
> SY24655, and I hope to receive your response.
> 
> Signed-off-by: Wenliang <wenliang202407@163.com>
> ---

Hi,

...

> @@ -103,7 +115,7 @@ static struct regmap_config ina2xx_regmap_config = {
>   	.val_bits = 16,
>   };
>   
> -enum ina2xx_ids { ina219, ina226 };
> +enum ina2xx_ids { ina219, ina226, sy24655};

Nitpick: Missing space before }

>   
>   struct ina2xx_config {
>   	u16 config_default;
> @@ -117,12 +129,13 @@ struct ina2xx_config {
>   
>   struct ina2xx_data {
>   	const struct ina2xx_config *config;
> -
> +
>   	long rshunt;
>   	long current_lsb_uA;
>   	long power_lsb_uW;
>   	struct mutex config_lock;
>   	struct regmap *regmap;
> +	struct i2c_client *client;
>   
>   	const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
>   };

...

> +static int sy24655_read_reg48(const struct i2c_client *client, u8 reg,
> +					long *accumulator_24, long *sample_count)
> +{
> +	u8 data[6];
> +	int err;

Maybe adding a blank line here?

> +	*accumulator_24 = 0;
> +	*sample_count = 0;
> +
> +	/* 48-bit register read */
> +	err = i2c_smbus_read_i2c_block_data(client, reg, 6, data);
> +	if (err < 0)
> +		return err;
> +	if (err != 6)
> +		return -EIO;
> +	*accumulator_24 = ((data[3] << 16) |
> +				(data[4] << 8) |
> +				data[5]);
> +	*sample_count = ((data[0] << 16) |
> +				(data[1] << 8) |
> +				data[2]);
> +
> +	return 0;
> +}
> +
> +static ssize_t sy24655_avg_power_show(struct device *dev,
> +					struct device_attribute *da, char *buf)
> +{
> +	struct ina2xx_data *data = dev_get_drvdata(dev);
> +	long sample_count, accumulator_24, regval;
> +	int status;
> +
> +	status = sy24655_read_reg48(data->client, SY24655_EIN,
> +						&accumulator_24, &sample_count);
> +	if (status)
> +		return status;
> +	regval = DIV_ROUND_CLOSEST(accumulator_24, sample_count);
> +	regval = regval * data->power_lsb_uW;
> +
> +

Nitpick: unneeded extra empty line

> +	return sysfs_emit(buf, "%li\n", regval);
> +}
> +
>   /* shunt voltage */
>   static SENSOR_DEVICE_ATTR_RO(in0_input, ina2xx_value, INA2XX_SHUNT_VOLTAGE);
>   /* shunt voltage over/under voltage alert setting and alarm */
> @@ -589,9 +659,13 @@ static SENSOR_DEVICE_ATTR_RO(power1_crit_alarm, ina226_alarm,
>   /* shunt resistance */
>   static SENSOR_DEVICE_ATTR_RW(shunt_resistor, ina2xx_shunt, INA2XX_CALIBRATION);
>   
> -/* update interval (ina226 only) */
> +/* update interval (ina226 and sy24655) */
>   static SENSOR_DEVICE_ATTR_RW(update_interval, ina226_interval, 0);
>   
> +/* calculate_avg_power (sy24655 only) */
> +static SENSOR_DEVICE_ATTR_RO(calculate_avg_power, sy24655_avg_power, 0);
> +
> +

Nitpick: unneeded extra empty line

>   /* pointers to created device attributes */
>   static struct attribute *ina2xx_attrs[] = {
>   	&sensor_dev_attr_in0_input.dev_attr.attr,
> @@ -624,6 +698,15 @@ static struct attribute *ina226_attrs[] = {
>   static const struct attribute_group ina226_group = {
>   	.attrs = ina226_attrs,
>   };


...

> @@ -691,10 +775,17 @@ static int ina2xx_probe(struct i2c_client *client)
>   		dev_err(dev, "error configuring the device: %d\n", ret);
>   		return -ENODEV;
>   	}
> -
> +	if (chip == sy24655)
> +		ret = sy24655_init(data);
> +	if (ret < 0) {
> +		dev_err(dev, "error configuring the accum_reg: %d\n", ret);
> +		return -ENODEV;

return ret;?

> +	}
>   	data->groups[group++] = &ina2xx_group;
>   	if (chip == ina226)
>   		data->groups[group++] = &ina226_group;
> +	else if (chip == sy24655)
> +		data->groups[group++] = &sy24655_group;
>   
>   	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
>   							   data, data->groups);
> @@ -713,6 +804,7 @@ static const struct i2c_device_id ina2xx_id[] = {
>   	{ "ina226", ina226 },
>   	{ "ina230", ina226 },
>   	{ "ina231", ina226 },
> +	{ "sy24655", sy24655},

Nitpick: missing space before }

>   	{ }
>   };
>   MODULE_DEVICE_TABLE(i2c, ina2xx_id);
> @@ -738,6 +830,10 @@ static const struct of_device_id __maybe_unused ina2xx_of_match[] = {
>   		.compatible = "ti,ina231",
>   		.data = (void *)ina226
>   	},
> +	{
> +		.compatible = "silergy,sy24655",
> +		.data = (void *)sy24655
> +	},
>   	{ },

Nitpick: Unrelated, but this comma could be removed.

CJ

>   };
>   MODULE_DEVICE_TABLE(of, ina2xx_of_match);

Re: [PATCH linux dev-6.11 1/2] v3: hwmon: modified ina2xx to match SY24655

[Guenter Roeck]

On 10/8/24 04:58, wenliang yan wrote:
> This change makes the INA2XX driver compatible with sy24655,
> 
> involving the reading of average power
> 

Your patches are corrupted. Also, please read and follow both
Documentation/process/submitting-patches.rst
and Documentation/hwmon/submitting-patches.rst.

The most important violations are in both subject and description
of your patches. Please use imperative mood as requested.

Guenter

Re: [PATCH linux dev 6.11] hwmon:add new hwmon driver sq52205

[kernel test robot]

Hi Wenliang,

kernel test robot noticed the following build errors:

[auto build test ERROR on linux/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Wenliang/hwmon-add-new-hwmon-driver-sq52205/20240826-103932
base:   linux/master
patch link:    https://lore.kernel.org/r/20240822074426.7241-1-wenliang202407%40163.com
patch subject: [PATCH linux dev 6.11] hwmon:add new hwmon driver sq52205
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20240826/202408261627.lm2xKacD-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240826/202408261627.lm2xKacD-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408261627.lm2xKacD-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/hwmon/sq52205.c:493:37: error: cannot assign to variable 'sq522xx_regmap_config' with const-qualified type 'const struct regmap_config'
     493 |         sq522xx_regmap_config.max_register = data->config->registers;
         |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
   drivers/hwmon/sq52205.c:67:35: note: variable 'sq522xx_regmap_config' declared const here
      67 | static const struct regmap_config sq522xx_regmap_config = {
         | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
      68 |         .reg_bits = 8,
         |         ~~~~~~~~~~~~~~
      69 |         .val_bits = 16,
         |         ~~~~~~~~~~~~~~~
      70 | };
         | ~
   1 error generated.


vim +493 drivers/hwmon/sq52205.c

   460	
   461	static int sq522xx_probe(struct i2c_client *client)
   462	{
   463		struct device *dev = &client->dev;
   464		struct sq522xx_data *data;
   465		struct device *hwmon_dev;
   466		u32 val;
   467		int ret, group = 0;
   468		enum sq522xx_ids chip;
   469	
   470		if (client->dev.of_node)
   471			chip = (uintptr_t)of_device_get_match_data(&client->dev);
   472		else
   473			chip = i2c_match_id(sq522xx_id, client)->driver_data;
   474	
   475		data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
   476		if (!data)
   477			return -ENOMEM;
   478	
   479		/* set the device type */
   480		data->client = client;
   481		data->config = &sq522xx_config[chip];
   482		mutex_init(&data->config_lock);
   483	
   484		if (of_property_read_u32(dev->of_node, "shunt-resistor", &val) < 0)
   485			val = SQ522XX_RSHUNT_DEFAULT;
   486	
   487	
   488		if (val <= 0 || val > data->config->calibration_factor)
   489			return -ENODEV;
   490	
   491		data->rshunt = val;
   492	
 > 493		sq522xx_regmap_config.max_register = data->config->registers;
   494	
   495		data->regmap = devm_regmap_init_i2c(client, &sq522xx_regmap_config);
   496		if (IS_ERR(data->regmap)) {
   497			dev_err(dev, "failed to allocate register map\n");
   498			return PTR_ERR(data->regmap);
   499		}
   500	
   501	
   502		ret = sq522xx_init(data);
   503		if (ret < 0) {
   504			dev_err(dev, "error configuring the device: %d\n", ret);
   505			return -ENODEV;
   506		}
   507		if (chip == sq52205) {
   508			ret = sq52205_init(data);
   509			if (ret < 0) {
   510				dev_err(dev, "error configuring the device cal: %d\n", ret);
   511				return -ENODEV;
   512			}
   513		}
   514	
   515		data->groups[group++] = &sq522xx_group;
   516		if (chip == sq52205)
   517			data->groups[group++] = &sq52205_group;
   518	
   519		hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
   520									data, data->groups);
   521		if (IS_ERR(hwmon_dev))
   522			return PTR_ERR(hwmon_dev);
   523	
   524		dev_info(dev, "power monitor %s (Rshunt = %li uOhm)\n",
   525			 client->name, data->rshunt);
   526	
   527		return 0;
   528	}
   529	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH linux dev 6.11] hwmon:add new hwmon driver sq52205

[kernel test robot]

Hi Wenliang,

kernel test robot noticed the following build errors:

[auto build test ERROR on linux/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Wenliang/hwmon-add-new-hwmon-driver-sq52205/20240826-103932
base:   linux/master
patch link:    https://lore.kernel.org/r/20240822074426.7241-1-wenliang202407%40163.com
patch subject: [PATCH linux dev 6.11] hwmon:add new hwmon driver sq52205
config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20240827/202408270025.5A0Q5KZO-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240827/202408270025.5A0Q5KZO-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408270025.5A0Q5KZO-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/hwmon/sq52205.c: In function 'sq522xx_probe':
>> drivers/hwmon/sq52205.c:493:44: error: assignment of member 'max_register' in read-only object
     493 |         sq522xx_regmap_config.max_register = data->config->registers;
         |                                            ^


vim +/max_register +493 drivers/hwmon/sq52205.c

   460	
   461	static int sq522xx_probe(struct i2c_client *client)
   462	{
   463		struct device *dev = &client->dev;
   464		struct sq522xx_data *data;
   465		struct device *hwmon_dev;
   466		u32 val;
   467		int ret, group = 0;
   468		enum sq522xx_ids chip;
   469	
   470		if (client->dev.of_node)
   471			chip = (uintptr_t)of_device_get_match_data(&client->dev);
   472		else
   473			chip = i2c_match_id(sq522xx_id, client)->driver_data;
   474	
   475		data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
   476		if (!data)
   477			return -ENOMEM;
   478	
   479		/* set the device type */
   480		data->client = client;
   481		data->config = &sq522xx_config[chip];
   482		mutex_init(&data->config_lock);
   483	
   484		if (of_property_read_u32(dev->of_node, "shunt-resistor", &val) < 0)
   485			val = SQ522XX_RSHUNT_DEFAULT;
   486	
   487	
   488		if (val <= 0 || val > data->config->calibration_factor)
   489			return -ENODEV;
   490	
   491		data->rshunt = val;
   492	
 > 493		sq522xx_regmap_config.max_register = data->config->registers;
   494	
   495		data->regmap = devm_regmap_init_i2c(client, &sq522xx_regmap_config);
   496		if (IS_ERR(data->regmap)) {
   497			dev_err(dev, "failed to allocate register map\n");
   498			return PTR_ERR(data->regmap);
   499		}
   500	
   501	
   502		ret = sq522xx_init(data);
   503		if (ret < 0) {
   504			dev_err(dev, "error configuring the device: %d\n", ret);
   505			return -ENODEV;
   506		}
   507		if (chip == sq52205) {
   508			ret = sq52205_init(data);
   509			if (ret < 0) {
   510				dev_err(dev, "error configuring the device cal: %d\n", ret);
   511				return -ENODEV;
   512			}
   513		}
   514	
   515		data->groups[group++] = &sq522xx_group;
   516		if (chip == sq52205)
   517			data->groups[group++] = &sq52205_group;
   518	
   519		hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
   520									data, data->groups);
   521		if (IS_ERR(hwmon_dev))
   522			return PTR_ERR(hwmon_dev);
   523	
   524		dev_info(dev, "power monitor %s (Rshunt = %li uOhm)\n",
   525			 client->name, data->rshunt);
   526	
   527		return 0;
   528	}
   529	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH linux dev 6.11] hwmon:add new hwmon driver sq52205

[kernel test robot]

Hi Wenliang,

kernel test robot noticed the following build errors:

[auto build test ERROR on linux/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Wenliang/hwmon-add-new-hwmon-driver-sq52205/20240912-002906
base:   linux/master
patch link:    https://lore.kernel.org/r/20240822074426.7241-1-wenliang202407%40163.com
patch subject: [PATCH linux dev 6.11] hwmon:add new hwmon driver sq52205
config: openrisc-randconfig-r072-20240913 (https://download.01.org/0day-ci/archive/20240913/202409131320.Ne0lQtTj-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240913/202409131320.Ne0lQtTj-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409131320.Ne0lQtTj-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/hwmon/sq52205.c: In function 'sq522xx_probe':
>> drivers/hwmon/sq52205.c:493:44: error: assignment of member 'max_register' in read-only object
     493 |         sq522xx_regmap_config.max_register = data->config->registers;
         |                                            ^


vim +/max_register +493 drivers/hwmon/sq52205.c

   460	
   461	static int sq522xx_probe(struct i2c_client *client)
   462	{
   463		struct device *dev = &client->dev;
   464		struct sq522xx_data *data;
   465		struct device *hwmon_dev;
   466		u32 val;
   467		int ret, group = 0;
   468		enum sq522xx_ids chip;
   469	
   470		if (client->dev.of_node)
   471			chip = (uintptr_t)of_device_get_match_data(&client->dev);
   472		else
   473			chip = i2c_match_id(sq522xx_id, client)->driver_data;
   474	
   475		data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
   476		if (!data)
   477			return -ENOMEM;
   478	
   479		/* set the device type */
   480		data->client = client;
   481		data->config = &sq522xx_config[chip];
   482		mutex_init(&data->config_lock);
   483	
   484		if (of_property_read_u32(dev->of_node, "shunt-resistor", &val) < 0)
   485			val = SQ522XX_RSHUNT_DEFAULT;
   486	
   487	
   488		if (val <= 0 || val > data->config->calibration_factor)
   489			return -ENODEV;
   490	
   491		data->rshunt = val;
   492	
 > 493		sq522xx_regmap_config.max_register = data->config->registers;
   494	
   495		data->regmap = devm_regmap_init_i2c(client, &sq522xx_regmap_config);
   496		if (IS_ERR(data->regmap)) {
   497			dev_err(dev, "failed to allocate register map\n");
   498			return PTR_ERR(data->regmap);
   499		}
   500	
   501	
   502		ret = sq522xx_init(data);
   503		if (ret < 0) {
   504			dev_err(dev, "error configuring the device: %d\n", ret);
   505			return -ENODEV;
   506		}
   507		if (chip == sq52205) {
   508			ret = sq52205_init(data);
   509			if (ret < 0) {
   510				dev_err(dev, "error configuring the device cal: %d\n", ret);
   511				return -ENODEV;
   512			}
   513		}
   514	
   515		data->groups[group++] = &sq522xx_group;
   516		if (chip == sq52205)
   517			data->groups[group++] = &sq52205_group;
   518	
   519		hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
   520									data, data->groups);
   521		if (IS_ERR(hwmon_dev))
   522			return PTR_ERR(hwmon_dev);
   523	
   524		dev_info(dev, "power monitor %s (Rshunt = %li uOhm)\n",
   525			 client->name, data->rshunt);
   526	
   527		return 0;
   528	}
   529	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH linux dev 6.11] hwmon:add new hwmon driver sq52205

[kernel test robot]

Hi Wenliang,

kernel test robot noticed the following build errors:

[auto build test ERROR on linux/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Wenliang/hwmon-add-new-hwmon-driver-sq52205/20240912-002906
base:   linux/master
patch link:    https://lore.kernel.org/r/20240822074426.7241-1-wenliang202407%40163.com
patch subject: [PATCH linux dev 6.11] hwmon:add new hwmon driver sq52205
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240914/202409140727.4pErU6oc-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project bf684034844c660b778f0eba103582f582b710c9)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/202409140727.4pErU6oc-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409140727.4pErU6oc-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/hwmon/sq52205.c:12:
   In file included from include/linux/i2c.h:19:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:25:
   In file included from include/linux/kernel_stat.h:8:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     548 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     561 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
         |                                                   ^
   In file included from drivers/hwmon/sq52205.c:12:
   In file included from include/linux/i2c.h:19:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:25:
   In file included from include/linux/kernel_stat.h:8:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     574 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   In file included from drivers/hwmon/sq52205.c:12:
   In file included from include/linux/i2c.h:19:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:25:
   In file included from include/linux/kernel_stat.h:8:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     585 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     595 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     605 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   In file included from drivers/hwmon/sq52205.c:12:
   In file included from include/linux/i2c.h:19:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:21:
   In file included from include/linux/mm.h:2232:
   include/linux/vmstat.h:517:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     517 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> drivers/hwmon/sq52205.c:493:37: error: cannot assign to variable 'sq522xx_regmap_config' with const-qualified type 'const struct regmap_config'
     493 |         sq522xx_regmap_config.max_register = data->config->registers;
         |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
   drivers/hwmon/sq52205.c:67:35: note: variable 'sq522xx_regmap_config' declared const here
      67 | static const struct regmap_config sq522xx_regmap_config = {
         | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
      68 |         .reg_bits = 8,
         |         ~~~~~~~~~~~~~~
      69 |         .val_bits = 16,
         |         ~~~~~~~~~~~~~~~
      70 | };
         | ~
   7 warnings and 1 error generated.


vim +493 drivers/hwmon/sq52205.c

   460	
   461	static int sq522xx_probe(struct i2c_client *client)
   462	{
   463		struct device *dev = &client->dev;
   464		struct sq522xx_data *data;
   465		struct device *hwmon_dev;
   466		u32 val;
   467		int ret, group = 0;
   468		enum sq522xx_ids chip;
   469	
   470		if (client->dev.of_node)
   471			chip = (uintptr_t)of_device_get_match_data(&client->dev);
   472		else
   473			chip = i2c_match_id(sq522xx_id, client)->driver_data;
   474	
   475		data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
   476		if (!data)
   477			return -ENOMEM;
   478	
   479		/* set the device type */
   480		data->client = client;
   481		data->config = &sq522xx_config[chip];
   482		mutex_init(&data->config_lock);
   483	
   484		if (of_property_read_u32(dev->of_node, "shunt-resistor", &val) < 0)
   485			val = SQ522XX_RSHUNT_DEFAULT;
   486	
   487	
   488		if (val <= 0 || val > data->config->calibration_factor)
   489			return -ENODEV;
   490	
   491		data->rshunt = val;
   492	
 > 493		sq522xx_regmap_config.max_register = data->config->registers;
   494	
   495		data->regmap = devm_regmap_init_i2c(client, &sq522xx_regmap_config);
   496		if (IS_ERR(data->regmap)) {
   497			dev_err(dev, "failed to allocate register map\n");
   498			return PTR_ERR(data->regmap);
   499		}
   500	
   501	
   502		ret = sq522xx_init(data);
   503		if (ret < 0) {
   504			dev_err(dev, "error configuring the device: %d\n", ret);
   505			return -ENODEV;
   506		}
   507		if (chip == sq52205) {
   508			ret = sq52205_init(data);
   509			if (ret < 0) {
   510				dev_err(dev, "error configuring the device cal: %d\n", ret);
   511				return -ENODEV;
   512			}
   513		}
   514	
   515		data->groups[group++] = &sq522xx_group;
   516		if (chip == sq52205)
   517			data->groups[group++] = &sq52205_group;
   518	
   519		hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
   520									data, data->groups);
   521		if (IS_ERR(hwmon_dev))
   522			return PTR_ERR(hwmon_dev);
   523	
   524		dev_info(dev, "power monitor %s (Rshunt = %li uOhm)\n",
   525			 client->name, data->rshunt);
   526	
   527		return 0;
   528	}
   529	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki