diff mbox series

[1/2] hwmon: ina3221: Add power sysfs nodes

Message ID 20180926064245.4091-2-nicoleotsuka@gmail.com (mailing list archive)
State Rejected
Headers show
Series hwmon: ina3221: Add power and enable sysfs nodes | expand

Commit Message

Nicolin Chen Sept. 26, 2018, 6:42 a.m. UTC
The hwmon sysfs ABI supports powerX_input and powerX_crit. This
can ease user space programs who care more about power in total
than voltage or current individually.

So this patch adds these two sysfs nodes for INA3221 driver.

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
 Documentation/hwmon/ina3221 |   4 +
 drivers/hwmon/ina3221.c     | 145 ++++++++++++++++++++++++++++++++----
 2 files changed, 133 insertions(+), 16 deletions(-)

Comments

Guenter Roeck Sept. 26, 2018, 12:34 p.m. UTC | #1
Hi Nicolin,

On 09/25/2018 11:42 PM, Nicolin Chen wrote:
> The hwmon sysfs ABI supports powerX_input and powerX_crit. This
> can ease user space programs who care more about power in total
> than voltage or current individually.
> 
> So this patch adds these two sysfs nodes for INA3221 driver.
> 

Ah, sorry, we can't do that. The sysfs nodes are for chips providing power
registers, not for kernel drivers to provide calculations based on voltage
and current measurements. Basic guideline is that we report what is there,
not some  calculation based on it.

This is even more true for power limits: We can not assume that the power limit
is (max voltage * max current). or (current voltage * max_current), or anything
else. We simply don't have the knowledge to make that assumption.

Thanks,
Guenter

> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> ---
>   Documentation/hwmon/ina3221 |   4 +
>   drivers/hwmon/ina3221.c     | 145 ++++++++++++++++++++++++++++++++----
>   2 files changed, 133 insertions(+), 16 deletions(-)
> 
> diff --git a/Documentation/hwmon/ina3221 b/Documentation/hwmon/ina3221
> index 2726038be5bd..6be64b553cd0 100644
> --- a/Documentation/hwmon/ina3221
> +++ b/Documentation/hwmon/ina3221
> @@ -34,3 +34,7 @@ curr[123]_max           Warning alert current(mA) setting, activates the
>                             average is above this value.
>   curr[123]_max_alarm     Warning alert current limit exceeded
>   in[456]_input           Shunt voltage(uV) for channels 1, 2, and 3 respectively
> +power[123]_input        Power(uW) measurement channels
> +power[123]_crit         Critical alert power(uW) setting, activates the
> +                          corresponding alarm when the respective power
> +                          is above this value
> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> index ce4d1f55e9cd..5890a2da3bfe 100644
> --- a/drivers/hwmon/ina3221.c
> +++ b/drivers/hwmon/ina3221.c
> @@ -38,6 +38,8 @@
>   #define INA3221_WARN3			0x0c
>   #define INA3221_MASK_ENABLE		0x0f
>   
> +#define INA3221_BUS(x)			(INA3221_BUS1 + ((x) << 1))
> +
>   #define INA3221_CONFIG_MODE_SHUNT	BIT(1)
>   #define INA3221_CONFIG_MODE_BUS		BIT(2)
>   #define INA3221_CONFIG_MODE_CONTINUOUS	BIT(3)
> @@ -172,43 +174,50 @@ static ssize_t ina3221_show_shunt_voltage(struct device *dev,
>   	return snprintf(buf, PAGE_SIZE, "%d\n", voltage_uv);
>   }
>   
> -static ssize_t ina3221_show_current(struct device *dev,
> -				    struct device_attribute *attr, char *buf)
> +static int __ina3221_show_current(struct ina3221_data *ina,
> +				  unsigned int reg, int *current_ma)
>   {
> -	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
> -	struct ina3221_data *ina = dev_get_drvdata(dev);
> -	unsigned int reg = sd_attr->index;
>   	unsigned int channel = register_channel[reg];
>   	struct ina3221_input *input = &ina->inputs[channel];
>   	int resistance_uo = input->shunt_resistor;
> -	int val, current_ma, voltage_nv, ret;
> +	int val, voltage_nv, ret;
>   
> +	/* Read bus shunt voltage */
>   	ret = ina3221_read_value(ina, reg, &val);
>   	if (ret)
>   		return ret;
> +
> +	/* LSB (4th bit) is 40 uV (40000 nV) */
>   	voltage_nv = val * 40000;
>   
> -	current_ma = DIV_ROUND_CLOSEST(voltage_nv, resistance_uo);
> +	*current_ma = DIV_ROUND_CLOSEST(voltage_nv, resistance_uo);
>   
> -	return snprintf(buf, PAGE_SIZE, "%d\n", current_ma);
> +	return 0;
>   }
>   
> -static ssize_t ina3221_set_current(struct device *dev,
> -				   struct device_attribute *attr,
> -				   const char *buf, size_t count)
> +static ssize_t ina3221_show_current(struct device *dev,
> +				    struct device_attribute *attr, char *buf)
>   {
>   	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
>   	struct ina3221_data *ina = dev_get_drvdata(dev);
>   	unsigned int reg = sd_attr->index;
> -	unsigned int channel = register_channel[reg];
> -	struct ina3221_input *input = &ina->inputs[channel];
> -	int resistance_uo = input->shunt_resistor;
> -	int val, current_ma, voltage_uv, ret;
> +	int current_ma, ret;
>   
> -	ret = kstrtoint(buf, 0, &current_ma);
> +	ret = __ina3221_show_current(ina, reg, &current_ma);
>   	if (ret)
>   		return ret;
>   
> +	return snprintf(buf, PAGE_SIZE, "%d\n", current_ma);
> +}
> +
> +static int __ina3221_set_current(struct ina3221_data *ina,
> +				 unsigned int reg, int current_ma)
> +{
> +	unsigned int channel = register_channel[reg];
> +	struct ina3221_input *input = &ina->inputs[channel];
> +	int resistance_uo = input->shunt_resistor;
> +	int val, voltage_uv, ret;
> +
>   	/* clamp current */
>   	current_ma = clamp_val(current_ma,
>   			       INT_MIN / resistance_uo,
> @@ -226,6 +235,26 @@ static ssize_t ina3221_set_current(struct device *dev,
>   	if (ret)
>   		return ret;
>   
> +	return 0;
> +}
> +
> +static ssize_t ina3221_set_current(struct device *dev,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
> +	struct ina3221_data *ina = dev_get_drvdata(dev);
> +	unsigned int reg = sd_attr->index;
> +	int current_ma, ret;
> +
> +	ret = kstrtoint(buf, 0, &current_ma);
> +	if (ret)
> +		return ret;
> +
> +	ret = __ina3221_set_current(ina, reg, current_ma);
> +	if (ret)
> +		return ret;
> +
>   	return count;
>   }
>   
> @@ -278,6 +307,68 @@ static ssize_t ina3221_show_alert(struct device *dev,
>   	return snprintf(buf, PAGE_SIZE, "%d\n", regval);
>   }
>   
> +static ssize_t ina3221_show_power(struct device *dev,
> +				  struct device_attribute *attr, char *buf)
> +{
> +	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
> +	struct ina3221_data *ina = dev_get_drvdata(dev);
> +	unsigned int reg = sd_attr->index;
> +	unsigned int channel = register_channel[reg];
> +	unsigned int reg_bus = INA3221_BUS(channel);
> +	int val, current_ma, voltage_mv, ret;
> +	s64 power_uw;
> +
> +	/* Read bus voltage */
> +	ret = ina3221_read_value(ina, reg_bus, &val);
> +	if (ret)
> +		return ret;
> +
> +	/* LSB (4th bit) is 8 mV */
> +	voltage_mv = val * 8;
> +
> +	/* Read calculated current */
> +	ret = __ina3221_show_current(ina, reg, &current_ma);
> +	if (ret)
> +		return ret;
> +
> +	power_uw = (s64)voltage_mv * current_ma;
> +
> +	return snprintf(buf, PAGE_SIZE, "%lld\n", power_uw);
> +}
> +
> +static ssize_t ina3221_set_power(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
> +	struct ina3221_data *ina = dev_get_drvdata(dev);
> +	unsigned int reg = sd_attr->index;
> +	unsigned int channel = register_channel[reg];
> +	unsigned int reg_bus = INA3221_BUS(channel);
> +	int val, current_ma, voltage_mv, ret;
> +	s64 power_uw;
> +
> +	ret = kstrtos64(buf, 0, &power_uw);
> +	if (ret)
> +		return ret;
> +
> +	/* Read bus voltage */
> +	ret = ina3221_read_value(ina, reg_bus, &val);
> +	if (ret)
> +		return ret;
> +
> +	/* LSB (4th bit) is 8 mV */
> +	voltage_mv = val * 8;
> +
> +	current_ma = DIV_ROUND_CLOSEST(power_uw, voltage_mv);
> +
> +	ret = __ina3221_set_current(ina, reg, current_ma);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
>   /* input channel label */
>   static SENSOR_DEVICE_ATTR(in1_label, 0444,
>   		ina3221_show_label, NULL, INA3221_CHANNEL1);
> @@ -350,6 +441,22 @@ static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO,
>   static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO,
>   		ina3221_show_shunt_voltage, NULL, INA3221_SHUNT3);
>   
> +/* calculated power */
> +static SENSOR_DEVICE_ATTR(power1_input, 0444,
> +		ina3221_show_power, NULL, INA3221_SHUNT1);
> +static SENSOR_DEVICE_ATTR(power2_input, 0444,
> +		ina3221_show_power, NULL, INA3221_SHUNT2);
> +static SENSOR_DEVICE_ATTR(power3_input, 0444,
> +		ina3221_show_power, NULL, INA3221_SHUNT3);
> +
> +/* critical power */
> +static SENSOR_DEVICE_ATTR(power1_crit, 0644,
> +		ina3221_show_power, ina3221_set_power, INA3221_CRIT1);
> +static SENSOR_DEVICE_ATTR(power2_crit, 0644,
> +		ina3221_show_power, ina3221_set_power, INA3221_CRIT2);
> +static SENSOR_DEVICE_ATTR(power3_crit, 0644,
> +		ina3221_show_power, ina3221_set_power, INA3221_CRIT3);
> +
>   static struct attribute *ina3221_attrs[] = {
>   	/* channel 1 -- make sure label at first */
>   	&sensor_dev_attr_in1_label.dev_attr.attr,
> @@ -361,6 +468,8 @@ static struct attribute *ina3221_attrs[] = {
>   	&sensor_dev_attr_curr1_max.dev_attr.attr,
>   	&sensor_dev_attr_curr1_max_alarm.dev_attr.attr,
>   	&sensor_dev_attr_in4_input.dev_attr.attr,
> +	&sensor_dev_attr_power1_input.dev_attr.attr,
> +	&sensor_dev_attr_power1_crit.dev_attr.attr,
>   
>   	/* channel 2 -- make sure label at first */
>   	&sensor_dev_attr_in2_label.dev_attr.attr,
> @@ -372,6 +481,8 @@ static struct attribute *ina3221_attrs[] = {
>   	&sensor_dev_attr_curr2_max.dev_attr.attr,
>   	&sensor_dev_attr_curr2_max_alarm.dev_attr.attr,
>   	&sensor_dev_attr_in5_input.dev_attr.attr,
> +	&sensor_dev_attr_power2_input.dev_attr.attr,
> +	&sensor_dev_attr_power2_crit.dev_attr.attr,
>   
>   	/* channel 3 -- make sure label at first */
>   	&sensor_dev_attr_in3_label.dev_attr.attr,
> @@ -383,6 +494,8 @@ static struct attribute *ina3221_attrs[] = {
>   	&sensor_dev_attr_curr3_max.dev_attr.attr,
>   	&sensor_dev_attr_curr3_max_alarm.dev_attr.attr,
>   	&sensor_dev_attr_in6_input.dev_attr.attr,
> +	&sensor_dev_attr_power3_input.dev_attr.attr,
> +	&sensor_dev_attr_power3_crit.dev_attr.attr,
>   
>   	NULL,
>   };
>
Nicolin Chen Sept. 26, 2018, 6:20 p.m. UTC | #2
On Wed, Sep 26, 2018 at 05:34:53AM -0700, Guenter Roeck wrote:
> Hi Nicolin,
> 
> On 09/25/2018 11:42 PM, Nicolin Chen wrote:
> > The hwmon sysfs ABI supports powerX_input and powerX_crit. This
> > can ease user space programs who care more about power in total
> > than voltage or current individually.
> > 
> > So this patch adds these two sysfs nodes for INA3221 driver.

> Ah, sorry, we can't do that. The sysfs nodes are for chips providing power
> registers, not for kernel drivers to provide calculations based on voltage
> and current measurements.

Hmm..I saw ina2xx.c and ltc4215.c are doing similar calculations...

> Basic guideline is that we report what is there, not some calculation based
> on it.

I could feel the back thoughts behind the guideline, but this does
give user space programs some trouble -- I have a few programs that
were used to read ina2xx driver which provides power nodes, but now
those programs will have to implement another function to read the
voltage and current separately to do further calculations.

Do you know any better solution for this situation?

> This is even more true for power limits: We can not assume that the power limit
> is (max voltage * max current). or (current voltage * max_current), or anything
> else. We simply don't have the knowledge to make that assumption.

I agree that power limit is a bit tricky here as the voltage could
change depending on the user space, Yes, I assumed that users who
set_power() should be aware of it (whether fixed or dynamical) so
as to decide to configure power limit or just current limit.

Thanks
Nicolin
Guenter Roeck Sept. 26, 2018, 7:45 p.m. UTC | #3
Hi Nicolin,

On Wed, Sep 26, 2018 at 11:20:06AM -0700, Nicolin Chen wrote:
> On Wed, Sep 26, 2018 at 05:34:53AM -0700, Guenter Roeck wrote:
> > Hi Nicolin,
> > 
> > On 09/25/2018 11:42 PM, Nicolin Chen wrote:
> > > The hwmon sysfs ABI supports powerX_input and powerX_crit. This
> > > can ease user space programs who care more about power in total
> > > than voltage or current individually.
> > > 
> > > So this patch adds these two sysfs nodes for INA3221 driver.
> 
> > Ah, sorry, we can't do that. The sysfs nodes are for chips providing power
> > registers, not for kernel drivers to provide calculations based on voltage
> > and current measurements.
> 
> Hmm..I saw ina2xx.c and ltc4215.c are doing similar calculations...
> 

ina2xx.c doesn't; the chips supported by the driver do have a register
reporting the power (0x03). ltc4215.c was not reviewed by a hwmon
maintainer. I think I mentioned before that you can find anything you want
in the Linux kernel. That doesn't make it right.

> > Basic guideline is that we report what is there, not some calculation based
> > on it.
> 
> I could feel the back thoughts behind the guideline, but this does
> give user space programs some trouble -- I have a few programs that
> were used to read ina2xx driver which provides power nodes, but now
> those programs will have to implement another function to read the
> voltage and current separately to do further calculations.
> 
> Do you know any better solution for this situation?
> 

Userspace simply must not assume that power attributes exist and calculate
it from voltage and current attributes if needed.

> > This is even more true for power limits: We can not assume that the power limit
> > is (max voltage * max current). or (current voltage * max_current), or anything
> > else. We simply don't have the knowledge to make that assumption.
> 
> I agree that power limit is a bit tricky here as the voltage could
> change depending on the user space, Yes, I assumed that users who
> set_power() should be aware of it (whether fixed or dynamical) so
> as to decide to configure power limit or just current limit.
> 

You just can not make any such assumptions, sorry. Limit attributes
absolutely must be reflected in hardware. 

Thanks,
Guenter
Nicolin Chen Sept. 26, 2018, 7:49 p.m. UTC | #4
Hello Guenter,

On Wed, Sep 26, 2018 at 12:45:34PM -0700, Guenter Roeck wrote:
> Hi Nicolin,
> 
> On Wed, Sep 26, 2018 at 11:20:06AM -0700, Nicolin Chen wrote:
> > On Wed, Sep 26, 2018 at 05:34:53AM -0700, Guenter Roeck wrote:
> > > Hi Nicolin,
> > > 
> > > On 09/25/2018 11:42 PM, Nicolin Chen wrote:
> > > > The hwmon sysfs ABI supports powerX_input and powerX_crit. This
> > > > can ease user space programs who care more about power in total
> > > > than voltage or current individually.
> > > > 
> > > > So this patch adds these two sysfs nodes for INA3221 driver.
> > 
> > > Ah, sorry, we can't do that. The sysfs nodes are for chips providing power
> > > registers, not for kernel drivers to provide calculations based on voltage
> > > and current measurements.
> > 
> > Hmm..I saw ina2xx.c and ltc4215.c are doing similar calculations...
> > 
> 
> ina2xx.c doesn't; the chips supported by the driver do have a register
> reporting the power (0x03). ltc4215.c was not reviewed by a hwmon
> maintainer. I think I mentioned before that you can find anything you want
> in the Linux kernel. That doesn't make it right.

OK. In that case, I am dropping this change.

Thanks
Nicolin
diff mbox series

Patch

diff --git a/Documentation/hwmon/ina3221 b/Documentation/hwmon/ina3221
index 2726038be5bd..6be64b553cd0 100644
--- a/Documentation/hwmon/ina3221
+++ b/Documentation/hwmon/ina3221
@@ -34,3 +34,7 @@  curr[123]_max           Warning alert current(mA) setting, activates the
                           average is above this value.
 curr[123]_max_alarm     Warning alert current limit exceeded
 in[456]_input           Shunt voltage(uV) for channels 1, 2, and 3 respectively
+power[123]_input        Power(uW) measurement channels
+power[123]_crit         Critical alert power(uW) setting, activates the
+                          corresponding alarm when the respective power
+                          is above this value
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index ce4d1f55e9cd..5890a2da3bfe 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -38,6 +38,8 @@ 
 #define INA3221_WARN3			0x0c
 #define INA3221_MASK_ENABLE		0x0f
 
+#define INA3221_BUS(x)			(INA3221_BUS1 + ((x) << 1))
+
 #define INA3221_CONFIG_MODE_SHUNT	BIT(1)
 #define INA3221_CONFIG_MODE_BUS		BIT(2)
 #define INA3221_CONFIG_MODE_CONTINUOUS	BIT(3)
@@ -172,43 +174,50 @@  static ssize_t ina3221_show_shunt_voltage(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%d\n", voltage_uv);
 }
 
-static ssize_t ina3221_show_current(struct device *dev,
-				    struct device_attribute *attr, char *buf)
+static int __ina3221_show_current(struct ina3221_data *ina,
+				  unsigned int reg, int *current_ma)
 {
-	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
-	struct ina3221_data *ina = dev_get_drvdata(dev);
-	unsigned int reg = sd_attr->index;
 	unsigned int channel = register_channel[reg];
 	struct ina3221_input *input = &ina->inputs[channel];
 	int resistance_uo = input->shunt_resistor;
-	int val, current_ma, voltage_nv, ret;
+	int val, voltage_nv, ret;
 
+	/* Read bus shunt voltage */
 	ret = ina3221_read_value(ina, reg, &val);
 	if (ret)
 		return ret;
+
+	/* LSB (4th bit) is 40 uV (40000 nV) */
 	voltage_nv = val * 40000;
 
-	current_ma = DIV_ROUND_CLOSEST(voltage_nv, resistance_uo);
+	*current_ma = DIV_ROUND_CLOSEST(voltage_nv, resistance_uo);
 
-	return snprintf(buf, PAGE_SIZE, "%d\n", current_ma);
+	return 0;
 }
 
-static ssize_t ina3221_set_current(struct device *dev,
-				   struct device_attribute *attr,
-				   const char *buf, size_t count)
+static ssize_t ina3221_show_current(struct device *dev,
+				    struct device_attribute *attr, char *buf)
 {
 	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
 	struct ina3221_data *ina = dev_get_drvdata(dev);
 	unsigned int reg = sd_attr->index;
-	unsigned int channel = register_channel[reg];
-	struct ina3221_input *input = &ina->inputs[channel];
-	int resistance_uo = input->shunt_resistor;
-	int val, current_ma, voltage_uv, ret;
+	int current_ma, ret;
 
-	ret = kstrtoint(buf, 0, &current_ma);
+	ret = __ina3221_show_current(ina, reg, &current_ma);
 	if (ret)
 		return ret;
 
+	return snprintf(buf, PAGE_SIZE, "%d\n", current_ma);
+}
+
+static int __ina3221_set_current(struct ina3221_data *ina,
+				 unsigned int reg, int current_ma)
+{
+	unsigned int channel = register_channel[reg];
+	struct ina3221_input *input = &ina->inputs[channel];
+	int resistance_uo = input->shunt_resistor;
+	int val, voltage_uv, ret;
+
 	/* clamp current */
 	current_ma = clamp_val(current_ma,
 			       INT_MIN / resistance_uo,
@@ -226,6 +235,26 @@  static ssize_t ina3221_set_current(struct device *dev,
 	if (ret)
 		return ret;
 
+	return 0;
+}
+
+static ssize_t ina3221_set_current(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
+	struct ina3221_data *ina = dev_get_drvdata(dev);
+	unsigned int reg = sd_attr->index;
+	int current_ma, ret;
+
+	ret = kstrtoint(buf, 0, &current_ma);
+	if (ret)
+		return ret;
+
+	ret = __ina3221_set_current(ina, reg, current_ma);
+	if (ret)
+		return ret;
+
 	return count;
 }
 
@@ -278,6 +307,68 @@  static ssize_t ina3221_show_alert(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%d\n", regval);
 }
 
+static ssize_t ina3221_show_power(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
+	struct ina3221_data *ina = dev_get_drvdata(dev);
+	unsigned int reg = sd_attr->index;
+	unsigned int channel = register_channel[reg];
+	unsigned int reg_bus = INA3221_BUS(channel);
+	int val, current_ma, voltage_mv, ret;
+	s64 power_uw;
+
+	/* Read bus voltage */
+	ret = ina3221_read_value(ina, reg_bus, &val);
+	if (ret)
+		return ret;
+
+	/* LSB (4th bit) is 8 mV */
+	voltage_mv = val * 8;
+
+	/* Read calculated current */
+	ret = __ina3221_show_current(ina, reg, &current_ma);
+	if (ret)
+		return ret;
+
+	power_uw = (s64)voltage_mv * current_ma;
+
+	return snprintf(buf, PAGE_SIZE, "%lld\n", power_uw);
+}
+
+static ssize_t ina3221_set_power(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
+	struct ina3221_data *ina = dev_get_drvdata(dev);
+	unsigned int reg = sd_attr->index;
+	unsigned int channel = register_channel[reg];
+	unsigned int reg_bus = INA3221_BUS(channel);
+	int val, current_ma, voltage_mv, ret;
+	s64 power_uw;
+
+	ret = kstrtos64(buf, 0, &power_uw);
+	if (ret)
+		return ret;
+
+	/* Read bus voltage */
+	ret = ina3221_read_value(ina, reg_bus, &val);
+	if (ret)
+		return ret;
+
+	/* LSB (4th bit) is 8 mV */
+	voltage_mv = val * 8;
+
+	current_ma = DIV_ROUND_CLOSEST(power_uw, voltage_mv);
+
+	ret = __ina3221_set_current(ina, reg, current_ma);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
 /* input channel label */
 static SENSOR_DEVICE_ATTR(in1_label, 0444,
 		ina3221_show_label, NULL, INA3221_CHANNEL1);
@@ -350,6 +441,22 @@  static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO,
 static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO,
 		ina3221_show_shunt_voltage, NULL, INA3221_SHUNT3);
 
+/* calculated power */
+static SENSOR_DEVICE_ATTR(power1_input, 0444,
+		ina3221_show_power, NULL, INA3221_SHUNT1);
+static SENSOR_DEVICE_ATTR(power2_input, 0444,
+		ina3221_show_power, NULL, INA3221_SHUNT2);
+static SENSOR_DEVICE_ATTR(power3_input, 0444,
+		ina3221_show_power, NULL, INA3221_SHUNT3);
+
+/* critical power */
+static SENSOR_DEVICE_ATTR(power1_crit, 0644,
+		ina3221_show_power, ina3221_set_power, INA3221_CRIT1);
+static SENSOR_DEVICE_ATTR(power2_crit, 0644,
+		ina3221_show_power, ina3221_set_power, INA3221_CRIT2);
+static SENSOR_DEVICE_ATTR(power3_crit, 0644,
+		ina3221_show_power, ina3221_set_power, INA3221_CRIT3);
+
 static struct attribute *ina3221_attrs[] = {
 	/* channel 1 -- make sure label at first */
 	&sensor_dev_attr_in1_label.dev_attr.attr,
@@ -361,6 +468,8 @@  static struct attribute *ina3221_attrs[] = {
 	&sensor_dev_attr_curr1_max.dev_attr.attr,
 	&sensor_dev_attr_curr1_max_alarm.dev_attr.attr,
 	&sensor_dev_attr_in4_input.dev_attr.attr,
+	&sensor_dev_attr_power1_input.dev_attr.attr,
+	&sensor_dev_attr_power1_crit.dev_attr.attr,
 
 	/* channel 2 -- make sure label at first */
 	&sensor_dev_attr_in2_label.dev_attr.attr,
@@ -372,6 +481,8 @@  static struct attribute *ina3221_attrs[] = {
 	&sensor_dev_attr_curr2_max.dev_attr.attr,
 	&sensor_dev_attr_curr2_max_alarm.dev_attr.attr,
 	&sensor_dev_attr_in5_input.dev_attr.attr,
+	&sensor_dev_attr_power2_input.dev_attr.attr,
+	&sensor_dev_attr_power2_crit.dev_attr.attr,
 
 	/* channel 3 -- make sure label at first */
 	&sensor_dev_attr_in3_label.dev_attr.attr,
@@ -383,6 +494,8 @@  static struct attribute *ina3221_attrs[] = {
 	&sensor_dev_attr_curr3_max.dev_attr.attr,
 	&sensor_dev_attr_curr3_max_alarm.dev_attr.attr,
 	&sensor_dev_attr_in6_input.dev_attr.attr,
+	&sensor_dev_attr_power3_input.dev_attr.attr,
+	&sensor_dev_attr_power3_crit.dev_attr.attr,
 
 	NULL,
 };