[PATCH v3 1/2] hwmon: modified ina2xx to match SY24655

[PATCH v3 1/2] hwmon: modified ina2xx to match SY24655

[Wenliang]

v3:Support the SY24655 for current and voltage detection as well as
power calculation.

SY24655 provides a power accumulator, thus adding the power1_average
parameter to output the average power, which can be used to calculate
energy; In order to achieve average power, adding extra EIN register
and ACCUM_CONFIG register addresses for SY24655. Due to the 48 bit
read-only nature of the EIN register, a function has been added
specifically for average power reading.


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



 Documentation/hwmon/ina2xx.rst | 25 ++++++++-
 drivers/hwmon/Kconfig          |  2 +-
 drivers/hwmon/ina2xx.c         | 97 ++++++++++++++++++++++++++++++++--
 3 files changed, 118 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 64fefb22ecee..48b446c366f2 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2178,7 +2178,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..9270af69ef6f 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -51,12 +51,20 @@
 #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
+
+#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 +92,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 +107,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;
@@ -127,7 +138,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 +160,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 +194,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 +508,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 +558,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 +770,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 +804,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 +911,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 +951,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 +990,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 +1020,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 v3 2/2] dt-bindings: modified ina2xx to match SY24655

[Wenliang]

Adding Silergy's sy24655 as an adapter chip for Ina2xx drivers.

Because it is similar to INA226, the supply voltage and pin definitions
are the same. It also supports IIC communication, with only two
additional registers added for configuring and calculating average power.

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..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 v3 2/2] dt-bindings: modified ina2xx to match SY24655

[Conor Dooley]

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

On Thu, Oct 24, 2024 at 04:30:55AM -0400, Wenliang wrote:
> Adding Silergy's sy24655 as an adapter chip for Ina2xx drivers.
> 
> Because it is similar to INA226, the supply voltage and pin definitions
> are the same. It also supports IIC communication, with only two
> additional registers added for configuring and calculating average power.
> 
> Signed-off-by: Wenliang <wenliang202407@163.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

The series has been send in response to an invalid message id:
https://lore.kernel.org/all/20241024083055.82047-2-wenliang202407@163.com/

Don't send new versions as replies to earlier versions, that tends to
get things lost in people's mailboxes.

Cheers,
Conor.

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

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

[Conor Dooley]

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

On Thu, Oct 24, 2024 at 05:32:19PM +0100, Conor Dooley wrote:
> On Thu, Oct 24, 2024 at 04:30:55AM -0400, Wenliang wrote:
> > Adding Silergy's sy24655 as an adapter chip for Ina2xx drivers.
> > 
> > Because it is similar to INA226, the supply voltage and pin definitions
> > are the same. It also supports IIC communication, with only two
> > additional registers added for configuring and calculating average power.
> > 
> > Signed-off-by: Wenliang <wenliang202407@163.com>
> 
> Acked-by: Conor Dooley <conor.dooley@microchip.com>

Actually, I take that back. You can add it if you send the patch to the
dt mailing list so it can be tested.

Thanks,
Conor.

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

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

[Guenter Roeck]

On 10/24/24 01:30, Wenliang wrote:
> Adding Silergy's sy24655 as an adapter chip for Ina2xx drivers.
> 
> Because it is similar to INA226, the supply voltage and pin definitions
> are the same. It also supports IIC communication, with only two
> additional registers added for configuring and calculating average power.
> 

Again, please consult
	Documentation/process/submitting-patches.rst
for both description and subject.

"dt-bindings: Add SY24655 to ina2xx devicetree bindings" or similar
would be a much better subject, and something like

"SY24655 is similar to INA226. Its supply voltage and pin definitions
are therefore the same. Compared to INA226, SY24655 has two additional
registers for configuring and calculating average power."

would be a much better patch description.

Also, as already requested by Conor, please copy the devicetree mailing list
on devicetree patches to enable automated testing.

Thanks,
Guenter

> 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..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

Re: [PATCH v3 1/2] hwmon: modified ina2xx to match SY24655

[Guenter Roeck]

On 10/24/24 01:30, Wenliang wrote:
> v3:Support the SY24655 for current and voltage detection as well as
> power calculation.
> 
> SY24655 provides a power accumulator, thus adding the power1_average
> parameter to output the average power, which can be used to calculate
> energy; In order to achieve average power, adding extra EIN register
> and ACCUM_CONFIG register addresses for SY24655. Due to the 48 bit
> read-only nature of the EIN register, a function has been added
> specifically for average power reading.
> 
Again, please consult
	Documentation/process/submitting-patches.rst
for both description and subject.

The subject should be something like

"hwmon: (ina226) Add support for SY24655".

The change log should be after "---".

The description needs to be in imperative mood. Something like

"""
SY24655 provides a power accumulator. Add support for the power1_average
attribute to report the average power.
"""

Implementation details are really irrelevant for the patch description.
I won't object if you add it, but please use imperative mood when doing so.
"adding" is not imperative mood.

> 
> Signed-off-by: Wenliang <wenliang202407@163.com>
> ---
> 
> 
> 
>   Documentation/hwmon/ina2xx.rst | 25 ++++++++-
>   drivers/hwmon/Kconfig          |  2 +-
>   drivers/hwmon/ina2xx.c         | 97 ++++++++++++++++++++++++++++++++--
>   3 files changed, 118 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

SY24655 supports both ...

> +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
> +------------------------------------------------

Does that pass generating the documentation ? Normally it wants
the "---" line to have the same length as the line above.

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

Drop "calculate".

> +======================= ====================================================
> +
>   .. note::
>   
>      - Configure `shunt_resistor` before configure `power1_crit`, because power
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 64fefb22ecee..48b446c366f2 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2178,7 +2178,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..9270af69ef6f 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -51,12 +51,20 @@
>   #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
> +
> +#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 +92,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 +107,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;
> @@ -127,7 +138,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 +160,8 @@ struct ina2xx_data {
>   	long power_lsb_uW;
>   	struct mutex config_lock;
>   	struct regmap *regmap;
> +	struct i2c_client *client;
> +

Unnecessary empty line.

>   };
>   
>   static const struct ina2xx_config ina2xx_config[] = {
> @@ -181,6 +194,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 +508,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;

I don't know what error code to return here, or if it makes sense to return an error
in the first place, but it is not "out of memory". If an error is returned,
the documentation needs to explain what the user is expected to do about it.
Just returning an error is not useful.

> +
> +	/* 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 +558,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 +770,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;

This is wrong. It must only return 0444 if the chip is sy24655
(or, to support other chips of the series at some later point,
if a flag such as has_power_average is added and set in struct
ina2xx_config).

>   		default:
>   			break;
>   		}
> @@ -734,7 +804,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 +911,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 +951,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 +990,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 +1020,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);
>   

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

[Krzysztof Kozlowski]

On 24/10/2024 10:30, Wenliang wrote:
> Adding Silergy's sy24655 as an adapter chip for Ina2xx drivers.
> 
> Because it is similar to INA226, the supply voltage and pin definitions
> are the same. It also supports IIC communication, with only two
> additional registers added for configuring and calculating average power.
> 
> Signed-off-by: Wenliang <wenliang202407@163.com>

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling.

Please kindly resend and include all necessary To/Cc entries.
</form letter>

Best regards,
Krzysztof

[PATCH linux dev-6.11 v3 1/2] hwmon: (ina226) Add support for SY24655

[Wenliang]

From: book <book@100ask.localdomain>

Signed-off-by: book <book@100ask.localdomain>
---

v3:Support the SY24655 for current and voltage detection as well as
power calculation.
SY24655 provides a power accumulator. Add support for the power1_average
attribute to report the average power. Add EIN register and
ACCUM_CONFIG register addresses for SY24655. Add a 48 bit read function
to meet the average power read.

 Documentation/hwmon/ina2xx.rst | 27 +++++++++-
 drivers/hwmon/Kconfig          |  2 +-
 drivers/hwmon/ina2xx.c         | 96 ++++++++++++++++++++++++++++++++--
 3 files changed, 118 insertions(+), 7 deletions(-)

diff --git a/Documentation/hwmon/ina2xx.rst b/Documentation/hwmon/ina2xx.rst
index 1ce161e6c0bf..a3860aae444c 100644
--- a/Documentation/hwmon/ina2xx.rst
+++ b/Documentation/hwmon/ina2xx.rst
@@ -63,6 +63,17 @@ 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 +96,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 supports 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,8 +124,8 @@ 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
+------------------------------------------------------------------------
 
 ======================= ====================================================
 curr1_lcrit		Critical low current
@@ -130,6 +146,13 @@ 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		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 85c34080a407..a55d9a88ce7d 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2178,7 +2178,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..16fdbc0eb1f9 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -51,12 +51,19 @@
 #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
+#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
@@ -97,6 +104,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;
@@ -127,12 +135,13 @@ 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;
 	bool has_alerts;	/* chip supports alerts and limits */
 	bool has_ishunt;	/* chip has internal shunt resistor */
+	bool has_power_average;	/* chip has internal shunt resistor */
 	int calibration_value;
 	int shunt_div;
 	int bus_voltage_shift;
@@ -149,6 +158,7 @@ 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[] = {
@@ -161,6 +171,7 @@ static const struct ina2xx_config ina2xx_config[] = {
 		.power_lsb_factor = 20,
 		.has_alerts = false,
 		.has_ishunt = false,
+		.has_power_average = false,
 	},
 	[ina226] = {
 		.config_default = INA226_CONFIG_DEFAULT,
@@ -171,6 +182,7 @@ static const struct ina2xx_config ina2xx_config[] = {
 		.power_lsb_factor = 25,
 		.has_alerts = true,
 		.has_ishunt = false,
+		.has_power_average = false,
 	},
 	[ina260] = {
 		.config_default = INA260_CONFIG_DEFAULT,
@@ -180,6 +192,18 @@ static const struct ina2xx_config ina2xx_config[] = {
 		.power_lsb_factor = 8,
 		.has_alerts = true,
 		.has_ishunt = true,
+		.has_power_average = false,
+	},
+	[sy24655] = {
+		.config_default = SY24655_CONFIG_DEFAULT,
+		.calibration_value = 4096,
+		.shunt_div = 400,
+		.bus_voltage_shift = 0,
+		.bus_voltage_lsb = 1250,
+		.power_lsb_factor = 25,
+		.has_alerts = true,
+		.has_ishunt = false,
+		.has_power_average = true,
 	},
 };
 
@@ -485,6 +509,42 @@ 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;
+
+	/* 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 +552,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);
@@ -651,6 +713,7 @@ static umode_t ina2xx_is_visible(const void *_data, enum hwmon_sensor_types type
 {
 	const struct ina2xx_data *data = _data;
 	bool has_alerts = data->config->has_alerts;
+	bool has_power_average = data->config->has_power_average;
 	enum ina2xx_ids chip = data->chip;
 
 	switch (type) {
@@ -668,6 +731,11 @@ static umode_t ina2xx_is_visible(const void *_data, enum hwmon_sensor_types type
 			if (has_alerts)
 				return 0444;
 			break;
+		case hwmon_power_average:
+			if (has_power_average)
+				return 0444;
+			break;
+			return 0444;
 		default:
 			break;
 		}
@@ -734,7 +802,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
 };
 
@@ -839,6 +908,19 @@ static int ina2xx_init(struct device *dev, struct ina2xx_data *data)
 				   INA226_ALERT_LATCH_ENABLE |
 						FIELD_PREP(INA226_ALERT_POLARITY, active_high));
 	}
+	if (data->config->has_power_average) {
+		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 +950,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 +989,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 +1019,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.47.0.229.g8f8d6eee53

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

[Wenliang]

From: book <book@100ask.localdomain>

SY24655 is similar to INA226. Its supply voltage and pin definitions
are therefore the same. Compared to INA226, SY24655 has two additional
registers for configuring and calculating average power.

Signed-off-by: book <book@100ask.localdomain>
Acked-by: Conor Dooley <conor.dooley@microchip.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..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.47.0.229.g8f8d6eee53

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

[Krzysztof Kozlowski]

On 03/11/2024 17:39, Wenliang wrote:
> From: book <book@100ask.localdomain>
> 
> SY24655 is similar to INA226. Its supply voltage and pin definitions
> are therefore the same. Compared to INA226, SY24655 has two additional
> registers for configuring and calculating average power.
> 
> Signed-off-by: book <book@100ask.localdomain>
> Acked-by: Conor Dooley <conor.dooley@microchip.com>

NAK, this never happened. If you think otherwise: provide proof, please.

Nothing improved in this binding, actually it got even worse with fake
email and probably name as well.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 03/11/2024 17:46, Krzysztof Kozlowski wrote:
> On 03/11/2024 17:39, Wenliang wrote:
>> From: book <book@100ask.localdomain>
>>
>> SY24655 is similar to INA226. Its supply voltage and pin definitions
>> are therefore the same. Compared to INA226, SY24655 has two additional
>> registers for configuring and calculating average power.
>>
>> Signed-off-by: book <book@100ask.localdomain>
>> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> 
> NAK, this never happened. If you think otherwise: provide proof, please.

Hm, now I found previous v3, so ack happened, but patch still has
incorrect author.

Please really carefully read submitting patches document, especially
parts about sending new versions, changelogs, subjects.


> 
> Nothing improved in this binding, actually it got even worse with fake
> email and probably name as well.


Best regards,
Krzysztof

Re: [PATCH linux dev-6.11 v3 1/2] hwmon: (ina226) Add support for SY24655

[Guenter Roeck]

On 11/3/24 08:39, Wenliang wrote:
> From: book <book@100ask.localdomain>
> 
> Signed-off-by: book <book@100ask.localdomain>
> ---
> 

There is still no appropriate description, and the Signed-off-by: tag
as well as the From: tag are unacceptable.

> v3:Support the SY24655 for current and voltage detection as well as
> power calculation.
> SY24655 provides a power accumulator. Add support for the power1_average
> attribute to report the average power. Add EIN register and
> ACCUM_CONFIG register addresses for SY24655. Add a 48 bit read function
> to meet the average power read.
> 

This does not describe the changes made in this version of the patch.

Guenter

>   Documentation/hwmon/ina2xx.rst | 27 +++++++++-
>   drivers/hwmon/Kconfig          |  2 +-
>   drivers/hwmon/ina2xx.c         | 96 ++++++++++++++++++++++++++++++++--
>   3 files changed, 118 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/hwmon/ina2xx.rst b/Documentation/hwmon/ina2xx.rst
> index 1ce161e6c0bf..a3860aae444c 100644
> --- a/Documentation/hwmon/ina2xx.rst
> +++ b/Documentation/hwmon/ina2xx.rst
> @@ -63,6 +63,17 @@ 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 +96,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 supports 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,8 +124,8 @@ 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
> +------------------------------------------------------------------------
>   
>   ======================= ====================================================
>   curr1_lcrit		Critical low current
> @@ -130,6 +146,13 @@ 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		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 85c34080a407..a55d9a88ce7d 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2178,7 +2178,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..16fdbc0eb1f9 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -51,12 +51,19 @@
>   #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
> +#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
> @@ -97,6 +104,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;
> @@ -127,12 +135,13 @@ 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;
>   	bool has_alerts;	/* chip supports alerts and limits */
>   	bool has_ishunt;	/* chip has internal shunt resistor */
> +	bool has_power_average;	/* chip has internal shunt resistor */
>   	int calibration_value;
>   	int shunt_div;
>   	int bus_voltage_shift;
> @@ -149,6 +158,7 @@ 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[] = {
> @@ -161,6 +171,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>   		.power_lsb_factor = 20,
>   		.has_alerts = false,
>   		.has_ishunt = false,
> +		.has_power_average = false,
>   	},
>   	[ina226] = {
>   		.config_default = INA226_CONFIG_DEFAULT,
> @@ -171,6 +182,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>   		.power_lsb_factor = 25,
>   		.has_alerts = true,
>   		.has_ishunt = false,
> +		.has_power_average = false,
>   	},
>   	[ina260] = {
>   		.config_default = INA260_CONFIG_DEFAULT,
> @@ -180,6 +192,18 @@ static const struct ina2xx_config ina2xx_config[] = {
>   		.power_lsb_factor = 8,
>   		.has_alerts = true,
>   		.has_ishunt = true,
> +		.has_power_average = false,
> +	},
> +	[sy24655] = {
> +		.config_default = SY24655_CONFIG_DEFAULT,
> +		.calibration_value = 4096,
> +		.shunt_div = 400,
> +		.bus_voltage_shift = 0,
> +		.bus_voltage_lsb = 1250,
> +		.power_lsb_factor = 25,
> +		.has_alerts = true,
> +		.has_ishunt = false,
> +		.has_power_average = true,
>   	},
>   };
>   
> @@ -485,6 +509,42 @@ 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;
> +
> +	/* 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 +552,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);
> @@ -651,6 +713,7 @@ static umode_t ina2xx_is_visible(const void *_data, enum hwmon_sensor_types type
>   {
>   	const struct ina2xx_data *data = _data;
>   	bool has_alerts = data->config->has_alerts;
> +	bool has_power_average = data->config->has_power_average;
>   	enum ina2xx_ids chip = data->chip;
>   
>   	switch (type) {
> @@ -668,6 +731,11 @@ static umode_t ina2xx_is_visible(const void *_data, enum hwmon_sensor_types type
>   			if (has_alerts)
>   				return 0444;
>   			break;
> +		case hwmon_power_average:
> +			if (has_power_average)
> +				return 0444;
> +			break;
> +			return 0444;
>   		default:
>   			break;
>   		}
> @@ -734,7 +802,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
>   };
>   
> @@ -839,6 +908,19 @@ static int ina2xx_init(struct device *dev, struct ina2xx_data *data)
>   				   INA226_ALERT_LATCH_ENABLE |
>   						FIELD_PREP(INA226_ALERT_POLARITY, active_high));
>   	}
> +	if (data->config->has_power_average) {
> +		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 +950,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 +989,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 +1019,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);
>   

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

[Conor Dooley]

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

On Sun, Nov 03, 2024 at 05:52:59PM +0100, Krzysztof Kozlowski wrote:
> On 03/11/2024 17:46, Krzysztof Kozlowski wrote:
> > On 03/11/2024 17:39, Wenliang wrote:
> >> From: book <book@100ask.localdomain>
> >>
> >> SY24655 is similar to INA226. Its supply voltage and pin definitions
> >> are therefore the same. Compared to INA226, SY24655 has two additional
> >> registers for configuring and calculating average power.
> >>
> >> Signed-off-by: book <book@100ask.localdomain>
> >> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> > 
> > NAK, this never happened. If you think otherwise: provide proof, please.
> 
> Hm, now I found previous v3, so ack happened, but patch still has
> incorrect author.

I didn't ack it with this weird authorship, so that needs to be fixed
before the ack can be applied.

> 
> Please really carefully read submitting patches document, especially
> parts about sending new versions, changelogs, subjects.
> 
> 
> > 
> > Nothing improved in this binding, actually it got even worse with fake
> > email and probably name as well.
> 
> 
> Best regards,
> Krzysztof
> 

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

[PATCH v3 1/2] hwmon: (ina226) Add support for SY24655

[wenliang]

SY24655: Support for current and voltage detection as well as
power calculation. 

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

ina2xx.rst: Add document content description for SY24655, including
datasheet, parameter description, and chip function description.

ina2xx.c: Add register addresses unique(SY24655_EIN and
SY24655_ACCUM_CONFIG) to SY24655 for data reading and initialization.
Add has_power_average in struct ina2xx_config to control average power
reading.
Add initialization data for SY24655.
Initialize the power accumulation register(SY24655_ACCUM_CONFIG)
for configuration SY24655.
Add a read function to the EIN register(48-bit reading).


 Documentation/hwmon/ina2xx.rst | 27 +++++++++-
 drivers/hwmon/Kconfig          |  2 +-
 drivers/hwmon/ina2xx.c         | 96 ++++++++++++++++++++++++++++++++--
 3 files changed, 118 insertions(+), 7 deletions(-)

diff --git a/Documentation/hwmon/ina2xx.rst b/Documentation/hwmon/ina2xx.rst
index 1ce161e6c0bf..a3860aae444c 100644
--- a/Documentation/hwmon/ina2xx.rst
+++ b/Documentation/hwmon/ina2xx.rst
@@ -63,6 +63,17 @@ 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 +96,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 supports 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,8 +124,8 @@ 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
+------------------------------------------------------------------------
 
 ======================= ====================================================
 curr1_lcrit		Critical low current
@@ -130,6 +146,13 @@ 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		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 cfb4e9314c62..a837b7a1cff4 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2189,7 +2189,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..16fdbc0eb1f9 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -51,12 +51,19 @@
 #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
+#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
@@ -97,6 +104,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;
@@ -127,12 +135,13 @@ 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;
 	bool has_alerts;	/* chip supports alerts and limits */
 	bool has_ishunt;	/* chip has internal shunt resistor */
+	bool has_power_average;	/* chip has internal shunt resistor */
 	int calibration_value;
 	int shunt_div;
 	int bus_voltage_shift;
@@ -149,6 +158,7 @@ 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[] = {
@@ -161,6 +171,7 @@ static const struct ina2xx_config ina2xx_config[] = {
 		.power_lsb_factor = 20,
 		.has_alerts = false,
 		.has_ishunt = false,
+		.has_power_average = false,
 	},
 	[ina226] = {
 		.config_default = INA226_CONFIG_DEFAULT,
@@ -171,6 +182,7 @@ static const struct ina2xx_config ina2xx_config[] = {
 		.power_lsb_factor = 25,
 		.has_alerts = true,
 		.has_ishunt = false,
+		.has_power_average = false,
 	},
 	[ina260] = {
 		.config_default = INA260_CONFIG_DEFAULT,
@@ -180,6 +192,18 @@ static const struct ina2xx_config ina2xx_config[] = {
 		.power_lsb_factor = 8,
 		.has_alerts = true,
 		.has_ishunt = true,
+		.has_power_average = false,
+	},
+	[sy24655] = {
+		.config_default = SY24655_CONFIG_DEFAULT,
+		.calibration_value = 4096,
+		.shunt_div = 400,
+		.bus_voltage_shift = 0,
+		.bus_voltage_lsb = 1250,
+		.power_lsb_factor = 25,
+		.has_alerts = true,
+		.has_ishunt = false,
+		.has_power_average = true,
 	},
 };
 
@@ -485,6 +509,42 @@ 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;
+
+	/* 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 +552,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);
@@ -651,6 +713,7 @@ static umode_t ina2xx_is_visible(const void *_data, enum hwmon_sensor_types type
 {
 	const struct ina2xx_data *data = _data;
 	bool has_alerts = data->config->has_alerts;
+	bool has_power_average = data->config->has_power_average;
 	enum ina2xx_ids chip = data->chip;
 
 	switch (type) {
@@ -668,6 +731,11 @@ static umode_t ina2xx_is_visible(const void *_data, enum hwmon_sensor_types type
 			if (has_alerts)
 				return 0444;
 			break;
+		case hwmon_power_average:
+			if (has_power_average)
+				return 0444;
+			break;
+			return 0444;
 		default:
 			break;
 		}
@@ -734,7 +802,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
 };
 
@@ -839,6 +908,19 @@ static int ina2xx_init(struct device *dev, struct ina2xx_data *data)
 				   INA226_ALERT_LATCH_ENABLE |
 						FIELD_PREP(INA226_ALERT_POLARITY, active_high));
 	}
+	if (data->config->has_power_average) {
+		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 +950,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 +989,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 +1019,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.47.0.229.g8f8d6eee53

[PATCH v3 2/2] dt-bindings: Add SY24655 to ina2xx devicetree bindings

[wenliang]

SY24655 is similar to INA226. Its supply voltage and pin definitions
are therefore the same. Compared to INA226, SY24655 has two additional
registers for configuring and calculating average power.

Signed-off-by: wenliang <wenliang202407@163.com>
Acked-by: Conor Dooley <conor.dooley@microchip.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..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.47.0.229.g8f8d6eee53

Re: [PATCH v3 1/2] hwmon: (ina226) Add support for SY24655

[Guenter Roeck]

On 11/6/24 07:05, wenliang wrote:
> SY24655: Support for current and voltage detection as well as
> power calculation.
> 
> Signed-off-by: wenliang <wenliang202407@163.com>

That isn't an acceptable signature. You used "Wenliang Yan" earlier.
I will use that name. That is borderline, but the alternative would really
to reject this patch set.

> ---
> 
> ina2xx.rst: Add document content description for SY24655, including
> datasheet, parameter description, and chip function description.
> 
> ina2xx.c: Add register addresses unique(SY24655_EIN and
> SY24655_ACCUM_CONFIG) to SY24655 for data reading and initialization.
> Add has_power_average in struct ina2xx_config to control average power
> reading.
> Add initialization data for SY24655.
> Initialize the power accumulation register(SY24655_ACCUM_CONFIG)
> for configuration SY24655.
> Add a read function to the EIN register(48-bit reading).
> 
> 

This is the _third_ v3 of your patch set, and the above is not an appropriate
change log. No need to resend, though; I'll fix up the problems myself.

However, _please_ spend some time reading the documents describing how
to submit patches into the Linux kernel or you seriously risk getting
future patch series rejected.

Guenter

>   Documentation/hwmon/ina2xx.rst | 27 +++++++++-
>   drivers/hwmon/Kconfig          |  2 +-
>   drivers/hwmon/ina2xx.c         | 96 ++++++++++++++++++++++++++++++++--
>   3 files changed, 118 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/hwmon/ina2xx.rst b/Documentation/hwmon/ina2xx.rst
> index 1ce161e6c0bf..a3860aae444c 100644
> --- a/Documentation/hwmon/ina2xx.rst
> +++ b/Documentation/hwmon/ina2xx.rst
> @@ -63,6 +63,17 @@ 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 +96,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 supports 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,8 +124,8 @@ 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
> +------------------------------------------------------------------------
>   
>   ======================= ====================================================
>   curr1_lcrit		Critical low current
> @@ -130,6 +146,13 @@ 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		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 cfb4e9314c62..a837b7a1cff4 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2189,7 +2189,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..16fdbc0eb1f9 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -51,12 +51,19 @@
>   #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
> +#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
> @@ -97,6 +104,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;
> @@ -127,12 +135,13 @@ 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;
>   	bool has_alerts;	/* chip supports alerts and limits */
>   	bool has_ishunt;	/* chip has internal shunt resistor */
> +	bool has_power_average;	/* chip has internal shunt resistor */
>   	int calibration_value;
>   	int shunt_div;
>   	int bus_voltage_shift;
> @@ -149,6 +158,7 @@ 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[] = {
> @@ -161,6 +171,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>   		.power_lsb_factor = 20,
>   		.has_alerts = false,
>   		.has_ishunt = false,
> +		.has_power_average = false,
>   	},
>   	[ina226] = {
>   		.config_default = INA226_CONFIG_DEFAULT,
> @@ -171,6 +182,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>   		.power_lsb_factor = 25,
>   		.has_alerts = true,
>   		.has_ishunt = false,
> +		.has_power_average = false,
>   	},
>   	[ina260] = {
>   		.config_default = INA260_CONFIG_DEFAULT,
> @@ -180,6 +192,18 @@ static const struct ina2xx_config ina2xx_config[] = {
>   		.power_lsb_factor = 8,
>   		.has_alerts = true,
>   		.has_ishunt = true,
> +		.has_power_average = false,
> +	},
> +	[sy24655] = {
> +		.config_default = SY24655_CONFIG_DEFAULT,
> +		.calibration_value = 4096,
> +		.shunt_div = 400,
> +		.bus_voltage_shift = 0,
> +		.bus_voltage_lsb = 1250,
> +		.power_lsb_factor = 25,
> +		.has_alerts = true,
> +		.has_ishunt = false,
> +		.has_power_average = true,
>   	},
>   };
>   
> @@ -485,6 +509,42 @@ 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;
> +
> +	/* 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 +552,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);
> @@ -651,6 +713,7 @@ static umode_t ina2xx_is_visible(const void *_data, enum hwmon_sensor_types type
>   {
>   	const struct ina2xx_data *data = _data;
>   	bool has_alerts = data->config->has_alerts;
> +	bool has_power_average = data->config->has_power_average;
>   	enum ina2xx_ids chip = data->chip;
>   
>   	switch (type) {
> @@ -668,6 +731,11 @@ static umode_t ina2xx_is_visible(const void *_data, enum hwmon_sensor_types type
>   			if (has_alerts)
>   				return 0444;
>   			break;
> +		case hwmon_power_average:
> +			if (has_power_average)
> +				return 0444;
> +			break;
> +			return 0444;
>   		default:
>   			break;
>   		}
> @@ -734,7 +802,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
>   };
>   
> @@ -839,6 +908,19 @@ static int ina2xx_init(struct device *dev, struct ina2xx_data *data)
>   				   INA226_ALERT_LATCH_ENABLE |
>   						FIELD_PREP(INA226_ALERT_POLARITY, active_high));
>   	}
> +	if (data->config->has_power_average) {
> +		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 +950,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 +989,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 +1019,11 @@ static const struct of_device_id __maybe_unused ina2xx_of_match[] = {
>   		.compatible = "ti,ina260",
>   		.data = (void *)ina260
>   	},
> -	{ },
> +	{
> +		.compatible = "silergy,sy24655",
> +		.data = (void *)sy24655
> +	},

Should be in alphabetic order.

> +	{ }
>   };
>   MODULE_DEVICE_TABLE(of, ina2xx_of_match);
>   

Re: [PATCH v3 1/2] hwmon: (ina226) Add support for SY24655

[Guenter Roeck]

On 11/6/24 07:05, wenliang wrote:
> SY24655: Support for current and voltage detection as well as
> power calculation.
> 
> Signed-off-by: wenliang <wenliang202407@163.com>
> ---

> @@ -668,6 +731,11 @@ static umode_t ina2xx_is_visible(const void *_data, enum hwmon_sensor_types type
>   			if (has_alerts)
>   				return 0444;
>   			break;
> +		case hwmon_power_average:
> +			if (has_power_average)
> +				return 0444;
> +			break;
> +			return 0444;

That is quite obviously wrong. Again, I'll fix it up myself
while applying the patch.

Guenter