Re: [PATCH linux dev-6.11 1/2] hwmon: modified ina2xx to match SY24655(SQ52205)
From: Guenter Roeck
Date: Wed Sep 11 2024 - 11:15:35 EST
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@xxxxxxx>
---
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@xxxxxxxxx>
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@xxxxxxxxx>
* 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);