* [PATCH v2 00/14] hwmon: (ina2xx) Cleanup and convert to use with_info API
@ 2024-08-30 1:05 Guenter Roeck
2024-08-30 1:05 ` [PATCH v2 01/14] hwmon: (ina2xx) Reorder include files to alphabetic order Guenter Roeck
` (13 more replies)
0 siblings, 14 replies; 24+ messages in thread
From: Guenter Roeck @ 2024-08-30 1:05 UTC (permalink / raw)
To: Hardware Monitoring; +Cc: Tzung-Bi Shih, Guenter Roeck
Clean up driver and convert to use with_info API to simplify driver
maintenance. Add support for reporting current limits.
v2:
- Added Reviewed-by: tags
- Fixed typos in commit description
- Dropped unrelated cosmetic changes
- Set .use_single_write and .use_single_read flags in regmap
configuration
- Set latch bit only once when initializing the chip
- Added support for current limit attributes
- Use shunt voltage register to calculate current
----------------------------------------------------------------
Guenter Roeck (14):
hwmon: (ina2xx) Reorder include files to alphabetic order
hwmon: (ina2xx) Replace platform data with device properties
hwmon: (ina2xx) Use bit operations
hwmon: (ina2xx) Mark regmap_config as const
hwmon: (ina2xx) Use local regmap pointer if used more than once
hwmon: (ina2xx) Re-initialize chip using regmap functions
hwmon: (ina2xx) Fix various overflow issues
hwmon: (ina2xx) Consolidate chip initialization code
hwmon: (ina2xx) Set alert latch
hwmon: (ina2xx) Move ina2xx_get_value()
hwmon: (ina2xx) Convert to use with_info hwmon API
hwmon: (ina2xx) Pass register to alert limit write functions
hwmon: (ina2xx) Add support for current limits
hwmon: (ina2xx) Use shunt voltage to calculate current
Documentation/hwmon/ina2xx.rst | 4 +
drivers/hwmon/ina2xx.c | 895 ++++++++++++++++++++++++-----------------
2 files changed, 531 insertions(+), 368 deletions(-)
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 01/14] hwmon: (ina2xx) Reorder include files to alphabetic order
2024-08-30 1:05 [PATCH v2 00/14] hwmon: (ina2xx) Cleanup and convert to use with_info API Guenter Roeck
@ 2024-08-30 1:05 ` Guenter Roeck
2024-08-30 1:05 ` [PATCH v2 02/14] hwmon: (ina2xx) Replace platform data with device properties Guenter Roeck
` (12 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Guenter Roeck @ 2024-08-30 1:05 UTC (permalink / raw)
To: Hardware Monitoring; +Cc: Tzung-Bi Shih, Guenter Roeck
Simplify driver maintenance by reordering include files to alphabetic
order.
Whule at it, drop unnecessary / unused jiffies.h.
No functional change.
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Added Reviewed-by: tag
drivers/hwmon/ina2xx.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 9ab4205622e2..a6a619a85eb6 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -22,19 +22,18 @@
* Thanks to Jan Volkering
*/
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/init.h>
+#include <linux/delay.h>
#include <linux/err.h>
-#include <linux/slab.h>
-#include <linux/i2c.h>
#include <linux/hwmon.h>
#include <linux/hwmon-sysfs.h>
-#include <linux/jiffies.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
#include <linux/of.h>
-#include <linux/delay.h>
-#include <linux/util_macros.h>
#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/util_macros.h>
#include <linux/platform_data/ina2xx.h>
--
2.45.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 02/14] hwmon: (ina2xx) Replace platform data with device properties
2024-08-30 1:05 [PATCH v2 00/14] hwmon: (ina2xx) Cleanup and convert to use with_info API Guenter Roeck
2024-08-30 1:05 ` [PATCH v2 01/14] hwmon: (ina2xx) Reorder include files to alphabetic order Guenter Roeck
@ 2024-08-30 1:05 ` Guenter Roeck
2024-08-30 1:05 ` [PATCH v2 03/14] hwmon: (ina2xx) Use bit operations Guenter Roeck
` (11 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Guenter Roeck @ 2024-08-30 1:05 UTC (permalink / raw)
To: Hardware Monitoring; +Cc: Tzung-Bi Shih, Guenter Roeck
There are no in-tree users of ina2xx platform data. Drop it and support
device properties instead as alternative if it should ever be needed.
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Added Reviewed-by: tag
drivers/hwmon/ina2xx.c | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index a6a619a85eb6..897657f8d685 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -30,13 +30,11 @@
#include <linux/init.h>
#include <linux/kernel.h>
#include <linux/module.h>
-#include <linux/of.h>
+#include <linux/property.h>
#include <linux/regmap.h>
#include <linux/slab.h>
#include <linux/util_macros.h>
-#include <linux/platform_data/ina2xx.h>
-
/* common register definitions */
#define INA2XX_CONFIG 0x00
#define INA2XX_SHUNT_VOLTAGE 0x01 /* readonly */
@@ -643,14 +641,8 @@ static int ina2xx_probe(struct i2c_client *client)
data->config = &ina2xx_config[chip];
mutex_init(&data->config_lock);
- if (of_property_read_u32(dev->of_node, "shunt-resistor", &val) < 0) {
- struct ina2xx_platform_data *pdata = dev_get_platdata(dev);
-
- if (pdata)
- val = pdata->shunt_uohms;
- else
- val = INA2XX_RSHUNT_DEFAULT;
- }
+ if (device_property_read_u32(dev, "shunt-resistor", &val) < 0)
+ val = INA2XX_RSHUNT_DEFAULT;
ina2xx_set_shunt(data, val);
@@ -667,7 +659,7 @@ static int ina2xx_probe(struct i2c_client *client)
return dev_err_probe(dev, ret, "failed to enable vs regulator\n");
if (chip == ina226) {
- if (of_property_read_bool(dev->of_node, "ti,alert-polarity-active-high")) {
+ if (device_property_read_bool(dev, "ti,alert-polarity-active-high")) {
ret = ina2xx_set_alert_polarity(data,
INA226_ALERT_POL_HIGH);
if (ret < 0) {
--
2.45.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 03/14] hwmon: (ina2xx) Use bit operations
2024-08-30 1:05 [PATCH v2 00/14] hwmon: (ina2xx) Cleanup and convert to use with_info API Guenter Roeck
2024-08-30 1:05 ` [PATCH v2 01/14] hwmon: (ina2xx) Reorder include files to alphabetic order Guenter Roeck
2024-08-30 1:05 ` [PATCH v2 02/14] hwmon: (ina2xx) Replace platform data with device properties Guenter Roeck
@ 2024-08-30 1:05 ` Guenter Roeck
2024-08-30 1:05 ` [PATCH v2 04/14] hwmon: (ina2xx) Mark regmap_config as const Guenter Roeck
` (10 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Guenter Roeck @ 2024-08-30 1:05 UTC (permalink / raw)
To: Hardware Monitoring; +Cc: Tzung-Bi Shih, Guenter Roeck
Use bit operations where possible to make the code more generic and to
align it with other drivers. Also use compile time conversion from bit
to mask to reduce runtime overhead.
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Added Reviewed-by: tag
drivers/hwmon/ina2xx.c | 84 +++++++++++++++++++++---------------------
1 file changed, 42 insertions(+), 42 deletions(-)
diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 897657f8d685..1b4170d02c94 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -22,6 +22,8 @@
* Thanks to Jan Volkering
*/
+#include <linux/bitfield.h>
+#include <linux/bits.h>
#include <linux/delay.h>
#include <linux/err.h>
#include <linux/hwmon.h>
@@ -65,25 +67,23 @@
#define INA2XX_RSHUNT_DEFAULT 10000
/* bit mask for reading the averaging setting in the configuration register */
-#define INA226_AVG_RD_MASK 0x0E00
+#define INA226_AVG_RD_MASK GENMASK(11, 9)
-#define INA226_READ_AVG(reg) (((reg) & INA226_AVG_RD_MASK) >> 9)
-#define INA226_SHIFT_AVG(val) ((val) << 9)
+#define INA226_READ_AVG(reg) FIELD_GET(INA226_AVG_RD_MASK, reg)
-#define INA226_ALERT_POLARITY_MASK 0x0002
-#define INA226_SHIFT_ALERT_POLARITY(val) ((val) << 1)
-#define INA226_ALERT_POL_LOW 0
-#define INA226_ALERT_POL_HIGH 1
+#define INA226_ALERT_POLARITY_MASK BIT(1)
+#define INA226_ALERT_POL_LOW 0
+#define INA226_ALERT_POL_HIGH 1
/* bit number of alert functions in Mask/Enable Register */
-#define INA226_SHUNT_OVER_VOLTAGE_BIT 15
-#define INA226_SHUNT_UNDER_VOLTAGE_BIT 14
-#define INA226_BUS_OVER_VOLTAGE_BIT 13
-#define INA226_BUS_UNDER_VOLTAGE_BIT 12
-#define INA226_POWER_OVER_LIMIT_BIT 11
+#define INA226_SHUNT_OVER_VOLTAGE_MASK BIT(15)
+#define INA226_SHUNT_UNDER_VOLTAGE_MASK BIT(14)
+#define INA226_BUS_OVER_VOLTAGE_MASK BIT(13)
+#define INA226_BUS_UNDER_VOLTAGE_MASK BIT(12)
+#define INA226_POWER_OVER_LIMIT_MASK BIT(11)
/* bit mask for alert config bits of Mask/Enable Register */
-#define INA226_ALERT_CONFIG_MASK 0xFC00
+#define INA226_ALERT_CONFIG_MASK GENMASK(15, 10)
#define INA226_ALERT_FUNCTION_FLAG BIT(4)
/* common attrs, ina226 attrs and NULL */
@@ -177,7 +177,7 @@ static u16 ina226_interval_to_reg(int interval)
avg_bits = find_closest(avg, ina226_avg_tab,
ARRAY_SIZE(ina226_avg_tab));
- return INA226_SHIFT_AVG(avg_bits);
+ return FIELD_PREP(INA226_AVG_RD_MASK, avg_bits);
}
static int ina2xx_set_alert_polarity(struct ina2xx_data *data,
@@ -185,7 +185,7 @@ static int ina2xx_set_alert_polarity(struct ina2xx_data *data,
{
return regmap_update_bits(data->regmap, INA226_MASK_ENABLE,
INA226_ALERT_POLARITY_MASK,
- INA226_SHIFT_ALERT_POLARITY(val));
+ FIELD_PREP(INA226_ALERT_POLARITY_MASK, val));
}
/*
@@ -322,20 +322,20 @@ static ssize_t ina2xx_value_show(struct device *dev,
return sysfs_emit(buf, "%d\n", ina2xx_get_value(data, attr->index, regval));
}
-static int ina226_reg_to_alert(struct ina2xx_data *data, u8 bit, u16 regval)
+static int ina226_reg_to_alert(struct ina2xx_data *data, u32 mask, u16 regval)
{
int reg;
- switch (bit) {
- case INA226_SHUNT_OVER_VOLTAGE_BIT:
- case INA226_SHUNT_UNDER_VOLTAGE_BIT:
+ switch (mask) {
+ case INA226_SHUNT_OVER_VOLTAGE_MASK:
+ case INA226_SHUNT_UNDER_VOLTAGE_MASK:
reg = INA2XX_SHUNT_VOLTAGE;
break;
- case INA226_BUS_OVER_VOLTAGE_BIT:
- case INA226_BUS_UNDER_VOLTAGE_BIT:
+ case INA226_BUS_OVER_VOLTAGE_MASK:
+ case INA226_BUS_UNDER_VOLTAGE_MASK:
reg = INA2XX_BUS_VOLTAGE;
break;
- case INA226_POWER_OVER_LIMIT_BIT:
+ case INA226_POWER_OVER_LIMIT_MASK:
reg = INA2XX_POWER;
break;
default:
@@ -351,19 +351,19 @@ static int ina226_reg_to_alert(struct ina2xx_data *data, u8 bit, u16 regval)
* Turns alert limit values into register values.
* Opposite of the formula in ina2xx_get_value().
*/
-static s16 ina226_alert_to_reg(struct ina2xx_data *data, u8 bit, int val)
+static s16 ina226_alert_to_reg(struct ina2xx_data *data, u32 mask, int val)
{
- switch (bit) {
- case INA226_SHUNT_OVER_VOLTAGE_BIT:
- case INA226_SHUNT_UNDER_VOLTAGE_BIT:
+ switch (mask) {
+ case INA226_SHUNT_OVER_VOLTAGE_MASK:
+ case INA226_SHUNT_UNDER_VOLTAGE_MASK:
val *= data->config->shunt_div;
return clamp_val(val, SHRT_MIN, SHRT_MAX);
- case INA226_BUS_OVER_VOLTAGE_BIT:
- case INA226_BUS_UNDER_VOLTAGE_BIT:
+ case INA226_BUS_OVER_VOLTAGE_MASK:
+ case INA226_BUS_UNDER_VOLTAGE_MASK:
val = (val * 1000) << data->config->bus_voltage_shift;
val = DIV_ROUND_CLOSEST(val, data->config->bus_voltage_lsb);
return clamp_val(val, 0, SHRT_MAX);
- case INA226_POWER_OVER_LIMIT_BIT:
+ case INA226_POWER_OVER_LIMIT_MASK:
val = DIV_ROUND_CLOSEST(val, data->power_lsb_uW);
return clamp_val(val, 0, USHRT_MAX);
default:
@@ -387,7 +387,7 @@ static ssize_t ina226_alert_show(struct device *dev,
if (ret)
goto abort;
- if (regval & BIT(attr->index)) {
+ if (regval & attr->index) {
ret = regmap_read(data->regmap, INA226_ALERT_LIMIT, ®val);
if (ret)
goto abort;
@@ -432,7 +432,7 @@ static ssize_t ina226_alert_store(struct device *dev,
if (val != 0) {
ret = regmap_update_bits(data->regmap, INA226_MASK_ENABLE,
INA226_ALERT_CONFIG_MASK,
- BIT(attr->index));
+ attr->index);
if (ret < 0)
goto abort;
}
@@ -456,7 +456,7 @@ static ssize_t ina226_alarm_show(struct device *dev,
if (ret)
return ret;
- alarm = (regval & BIT(attr->index)) &&
+ alarm = (regval & attr->index) &&
(regval & INA226_ALERT_FUNCTION_FLAG);
return sysfs_emit(buf, "%d\n", alarm);
}
@@ -552,25 +552,25 @@ static ssize_t ina226_interval_show(struct device *dev,
static SENSOR_DEVICE_ATTR_RO(in0_input, ina2xx_value, INA2XX_SHUNT_VOLTAGE);
/* shunt voltage over/under voltage alert setting and alarm */
static SENSOR_DEVICE_ATTR_RW(in0_crit, ina226_alert,
- INA226_SHUNT_OVER_VOLTAGE_BIT);
+ INA226_SHUNT_OVER_VOLTAGE_MASK);
static SENSOR_DEVICE_ATTR_RW(in0_lcrit, ina226_alert,
- INA226_SHUNT_UNDER_VOLTAGE_BIT);
+ INA226_SHUNT_UNDER_VOLTAGE_MASK);
static SENSOR_DEVICE_ATTR_RO(in0_crit_alarm, ina226_alarm,
- INA226_SHUNT_OVER_VOLTAGE_BIT);
+ INA226_SHUNT_OVER_VOLTAGE_MASK);
static SENSOR_DEVICE_ATTR_RO(in0_lcrit_alarm, ina226_alarm,
- INA226_SHUNT_UNDER_VOLTAGE_BIT);
+ INA226_SHUNT_UNDER_VOLTAGE_MASK);
/* bus voltage */
static SENSOR_DEVICE_ATTR_RO(in1_input, ina2xx_value, INA2XX_BUS_VOLTAGE);
/* bus voltage over/under voltage alert setting and alarm */
static SENSOR_DEVICE_ATTR_RW(in1_crit, ina226_alert,
- INA226_BUS_OVER_VOLTAGE_BIT);
+ INA226_BUS_OVER_VOLTAGE_MASK);
static SENSOR_DEVICE_ATTR_RW(in1_lcrit, ina226_alert,
- INA226_BUS_UNDER_VOLTAGE_BIT);
+ INA226_BUS_UNDER_VOLTAGE_MASK);
static SENSOR_DEVICE_ATTR_RO(in1_crit_alarm, ina226_alarm,
- INA226_BUS_OVER_VOLTAGE_BIT);
+ INA226_BUS_OVER_VOLTAGE_MASK);
static SENSOR_DEVICE_ATTR_RO(in1_lcrit_alarm, ina226_alarm,
- INA226_BUS_UNDER_VOLTAGE_BIT);
+ INA226_BUS_UNDER_VOLTAGE_MASK);
/* calculated current */
static SENSOR_DEVICE_ATTR_RO(curr1_input, ina2xx_value, INA2XX_CURRENT);
@@ -579,9 +579,9 @@ static SENSOR_DEVICE_ATTR_RO(curr1_input, ina2xx_value, INA2XX_CURRENT);
static SENSOR_DEVICE_ATTR_RO(power1_input, ina2xx_value, INA2XX_POWER);
/* over-limit power alert setting and alarm */
static SENSOR_DEVICE_ATTR_RW(power1_crit, ina226_alert,
- INA226_POWER_OVER_LIMIT_BIT);
+ INA226_POWER_OVER_LIMIT_MASK);
static SENSOR_DEVICE_ATTR_RO(power1_crit_alarm, ina226_alarm,
- INA226_POWER_OVER_LIMIT_BIT);
+ INA226_POWER_OVER_LIMIT_MASK);
/* shunt resistance */
static SENSOR_DEVICE_ATTR_RW(shunt_resistor, ina2xx_shunt, INA2XX_CALIBRATION);
--
2.45.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 04/14] hwmon: (ina2xx) Mark regmap_config as const
2024-08-30 1:05 [PATCH v2 00/14] hwmon: (ina2xx) Cleanup and convert to use with_info API Guenter Roeck
` (2 preceding siblings ...)
2024-08-30 1:05 ` [PATCH v2 03/14] hwmon: (ina2xx) Use bit operations Guenter Roeck
@ 2024-08-30 1:05 ` Guenter Roeck
2024-08-30 12:28 ` Tzung-Bi Shih
2024-08-30 1:05 ` [PATCH v2 05/14] hwmon: (ina2xx) Use local regmap pointer if used more than once Guenter Roeck
` (9 subsequent siblings)
13 siblings, 1 reply; 24+ messages in thread
From: Guenter Roeck @ 2024-08-30 1:05 UTC (permalink / raw)
To: Hardware Monitoring; +Cc: Tzung-Bi Shih, Guenter Roeck
Recent versions of checkpatch complain that struct regmap_config
should be declared as const.
WARNING: struct regmap_config should normally be const
Doing so reveals a potential problem in the driver: If both supported
chips are present in a single system, the maximum number of registers
may race when devices are instantiated since max_registers is updated
in the probe function. Solve the problem by setting .max_registers to the
maximum register address of all supported chips. This does not make a
practical difference while fixing the potential race condition and reducing
code complexity.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Fixed typo in patch description (devic es -> devices)
drivers/hwmon/ina2xx.c | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 1b4170d02c94..9d93190874d7 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -50,10 +50,6 @@
#define INA226_ALERT_LIMIT 0x07
#define INA226_DIE_ID 0xFF
-/* register count */
-#define INA219_REGISTERS 6
-#define INA226_REGISTERS 8
-
#define INA2XX_MAX_REGISTERS 8
/* settings - depend on use case */
@@ -95,9 +91,10 @@
*/
#define INA226_TOTAL_CONV_TIME_DEFAULT 2200
-static struct regmap_config ina2xx_regmap_config = {
+static const struct regmap_config ina2xx_regmap_config = {
.reg_bits = 8,
.val_bits = 16,
+ .max_register = INA2XX_MAX_REGISTERS,
};
enum ina2xx_ids { ina219, ina226 };
@@ -105,7 +102,6 @@ enum ina2xx_ids { ina219, ina226 };
struct ina2xx_config {
u16 config_default;
int calibration_value;
- int registers;
int shunt_div;
int bus_voltage_shift;
int bus_voltage_lsb; /* uV */
@@ -128,7 +124,6 @@ static const struct ina2xx_config ina2xx_config[] = {
[ina219] = {
.config_default = INA219_CONFIG_DEFAULT,
.calibration_value = 4096,
- .registers = INA219_REGISTERS,
.shunt_div = 100,
.bus_voltage_shift = 3,
.bus_voltage_lsb = 4000,
@@ -137,7 +132,6 @@ static const struct ina2xx_config ina2xx_config[] = {
[ina226] = {
.config_default = INA226_CONFIG_DEFAULT,
.calibration_value = 2048,
- .registers = INA226_REGISTERS,
.shunt_div = 400,
.bus_voltage_shift = 0,
.bus_voltage_lsb = 1250,
@@ -646,8 +640,6 @@ static int ina2xx_probe(struct i2c_client *client)
ina2xx_set_shunt(data, val);
- ina2xx_regmap_config.max_register = data->config->registers;
-
data->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
if (IS_ERR(data->regmap)) {
dev_err(dev, "failed to allocate register map\n");
--
2.45.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 05/14] hwmon: (ina2xx) Use local regmap pointer if used more than once
2024-08-30 1:05 [PATCH v2 00/14] hwmon: (ina2xx) Cleanup and convert to use with_info API Guenter Roeck
` (3 preceding siblings ...)
2024-08-30 1:05 ` [PATCH v2 04/14] hwmon: (ina2xx) Mark regmap_config as const Guenter Roeck
@ 2024-08-30 1:05 ` Guenter Roeck
2024-08-30 1:05 ` [PATCH v2 06/14] hwmon: (ina2xx) Re-initialize chip using regmap functions Guenter Roeck
` (8 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Guenter Roeck @ 2024-08-30 1:05 UTC (permalink / raw)
To: Hardware Monitoring; +Cc: Tzung-Bi Shih, Guenter Roeck
If regmap is accessed more than once in a function, declare and used
local regmap variable.
While at it, drop low value debug messages.
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Added Reviewed-by: tag
drivers/hwmon/ina2xx.c | 23 ++++++++++-------------
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 9d93190874d7..ed8764a29d3f 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -210,18 +210,14 @@ static int ina2xx_init(struct ina2xx_data *data)
static int ina2xx_read_reg(struct device *dev, int reg, unsigned int *regval)
{
struct ina2xx_data *data = dev_get_drvdata(dev);
+ struct regmap *regmap = data->regmap;
int ret, retry;
- dev_dbg(dev, "Starting register %d read\n", reg);
-
for (retry = 5; retry; retry--) {
-
- ret = regmap_read(data->regmap, reg, regval);
+ ret = regmap_read(regmap, reg, regval);
if (ret < 0)
return ret;
- dev_dbg(dev, "read %d, val = 0x%04x\n", reg, *regval);
-
/*
* If the current value in the calibration register is 0, the
* power and current registers will also remain at 0. In case
@@ -233,8 +229,7 @@ static int ina2xx_read_reg(struct device *dev, int reg, unsigned int *regval)
if (*regval == 0) {
unsigned int cal;
- ret = regmap_read(data->regmap, INA2XX_CALIBRATION,
- &cal);
+ ret = regmap_read(regmap, INA2XX_CALIBRATION, &cal);
if (ret < 0)
return ret;
@@ -372,17 +367,18 @@ static ssize_t ina226_alert_show(struct device *dev,
{
struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
struct ina2xx_data *data = dev_get_drvdata(dev);
+ struct regmap *regmap = data->regmap;
int regval;
int val = 0;
int ret;
mutex_lock(&data->config_lock);
- ret = regmap_read(data->regmap, INA226_MASK_ENABLE, ®val);
+ ret = regmap_read(regmap, INA226_MASK_ENABLE, ®val);
if (ret)
goto abort;
if (regval & attr->index) {
- ret = regmap_read(data->regmap, INA226_ALERT_LIMIT, ®val);
+ ret = regmap_read(regmap, INA226_ALERT_LIMIT, ®val);
if (ret)
goto abort;
val = ina226_reg_to_alert(data, attr->index, regval);
@@ -400,6 +396,7 @@ static ssize_t ina226_alert_store(struct device *dev,
{
struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
struct ina2xx_data *data = dev_get_drvdata(dev);
+ struct regmap *regmap = data->regmap;
unsigned long val;
int ret;
@@ -413,18 +410,18 @@ static ssize_t ina226_alert_store(struct device *dev,
* if the value is non-zero.
*/
mutex_lock(&data->config_lock);
- ret = regmap_update_bits(data->regmap, INA226_MASK_ENABLE,
+ ret = regmap_update_bits(regmap, INA226_MASK_ENABLE,
INA226_ALERT_CONFIG_MASK, 0);
if (ret < 0)
goto abort;
- ret = regmap_write(data->regmap, INA226_ALERT_LIMIT,
+ ret = regmap_write(regmap, INA226_ALERT_LIMIT,
ina226_alert_to_reg(data, attr->index, val));
if (ret < 0)
goto abort;
if (val != 0) {
- ret = regmap_update_bits(data->regmap, INA226_MASK_ENABLE,
+ ret = regmap_update_bits(regmap, INA226_MASK_ENABLE,
INA226_ALERT_CONFIG_MASK,
attr->index);
if (ret < 0)
--
2.45.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 06/14] hwmon: (ina2xx) Re-initialize chip using regmap functions
2024-08-30 1:05 [PATCH v2 00/14] hwmon: (ina2xx) Cleanup and convert to use with_info API Guenter Roeck
` (4 preceding siblings ...)
2024-08-30 1:05 ` [PATCH v2 05/14] hwmon: (ina2xx) Use local regmap pointer if used more than once Guenter Roeck
@ 2024-08-30 1:05 ` Guenter Roeck
2024-08-30 12:29 ` Tzung-Bi Shih
2024-08-30 1:05 ` [PATCH v2 07/14] hwmon: (ina2xx) Fix various overflow issues Guenter Roeck
` (7 subsequent siblings)
13 siblings, 1 reply; 24+ messages in thread
From: Guenter Roeck @ 2024-08-30 1:05 UTC (permalink / raw)
To: Hardware Monitoring; +Cc: Tzung-Bi Shih, Guenter Roeck
If it is necessary to re-initialize the chip, for example because
it has been power cycled, use regmap functions to update register
contents. This ensures that all registers, including the configuration
register and alert registers, are updated to previously configured
values without having to locally cache everything.
For this to work, volatile registers have to be marked as volatile.
Also, the cache needs to be bypassed when reading the calibration
and mask_enable registers. While the calibration register is not
volatile, it will be reset to 0 if the chip has been power cycled.
Most of the bits in the mask_enable register are configuration bits,
except for bit 4 which reports if an alert has been observed.
Both registers need to be marked as non-volatile to be updated
after a power cycle, but it is necessary to bypass the cache when
reading them to detect if the chip has been power cycled and to
read the alert status.
The chip does not support register auto-increments. It is therefore
necessary to configure regmap to use single register read/write
operations. Otherwise regmap tries to write all registers in a single
operation when synchronizing register contents with the hardware,
and the synchronization fails.
Another necessary change is to declare ina226_alert_to_reg() as u16.
So far it returned an s16 which is sign extended to a large negative
value which is then sent to regmap as unsigned int, causing an -EINVAL
error return.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Fixed typo in patch description (ben -> been)
Added .use_single_write = true and .use_single_read = true to
regmap configuration
drivers/hwmon/ina2xx.c | 50 ++++++++++++++++++++++++++++++++++--------
1 file changed, 41 insertions(+), 9 deletions(-)
diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index ed8764a29d3f..db6432523e59 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -91,10 +91,41 @@
*/
#define INA226_TOTAL_CONV_TIME_DEFAULT 2200
+static bool ina2xx_writeable_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case INA2XX_CONFIG:
+ case INA2XX_CALIBRATION:
+ case INA226_MASK_ENABLE:
+ case INA226_ALERT_LIMIT:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static bool ina2xx_volatile_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case INA2XX_SHUNT_VOLTAGE:
+ case INA2XX_BUS_VOLTAGE:
+ case INA2XX_POWER:
+ case INA2XX_CURRENT:
+ return true;
+ default:
+ return false;
+ }
+}
+
static const struct regmap_config ina2xx_regmap_config = {
.reg_bits = 8,
.val_bits = 16,
+ .use_single_write = true,
+ .use_single_read = true,
.max_register = INA2XX_MAX_REGISTERS,
+ .cache_type = REGCACHE_MAPLE,
+ .volatile_reg = ina2xx_volatile_reg,
+ .writeable_reg = ina2xx_writeable_reg,
};
enum ina2xx_ids { ina219, ina226 };
@@ -229,16 +260,16 @@ static int ina2xx_read_reg(struct device *dev, int reg, unsigned int *regval)
if (*regval == 0) {
unsigned int cal;
- ret = regmap_read(regmap, INA2XX_CALIBRATION, &cal);
+ ret = regmap_read_bypassed(regmap, INA2XX_CALIBRATION, &cal);
if (ret < 0)
return ret;
if (cal == 0) {
dev_warn(dev, "chip not calibrated, reinitializing\n");
- ret = ina2xx_init(data);
- if (ret < 0)
- return ret;
+ regcache_mark_dirty(regmap);
+ regcache_sync(regmap);
+
/*
* Let's make sure the power and current
* registers have been updated before trying
@@ -340,7 +371,7 @@ static int ina226_reg_to_alert(struct ina2xx_data *data, u32 mask, u16 regval)
* Turns alert limit values into register values.
* Opposite of the formula in ina2xx_get_value().
*/
-static s16 ina226_alert_to_reg(struct ina2xx_data *data, u32 mask, int val)
+static u16 ina226_alert_to_reg(struct ina2xx_data *data, u32 mask, int val)
{
switch (mask) {
case INA226_SHUNT_OVER_VOLTAGE_MASK:
@@ -439,16 +470,17 @@ static ssize_t ina226_alarm_show(struct device *dev,
{
struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
struct ina2xx_data *data = dev_get_drvdata(dev);
- int regval;
+ unsigned int mask;
int alarm = 0;
int ret;
- ret = regmap_read(data->regmap, INA226_MASK_ENABLE, ®val);
+ ret = regmap_read_bypassed(data->regmap, INA226_MASK_ENABLE, &mask);
if (ret)
return ret;
- alarm = (regval & attr->index) &&
- (regval & INA226_ALERT_FUNCTION_FLAG);
+ alarm = (mask & attr->index) &&
+ (mask & INA226_ALERT_FUNCTION_FLAG);
+
return sysfs_emit(buf, "%d\n", alarm);
}
--
2.45.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 07/14] hwmon: (ina2xx) Fix various overflow issues
2024-08-30 1:05 [PATCH v2 00/14] hwmon: (ina2xx) Cleanup and convert to use with_info API Guenter Roeck
` (5 preceding siblings ...)
2024-08-30 1:05 ` [PATCH v2 06/14] hwmon: (ina2xx) Re-initialize chip using regmap functions Guenter Roeck
@ 2024-08-30 1:05 ` Guenter Roeck
2024-08-30 1:05 ` [PATCH v2 08/14] hwmon: (ina2xx) Consolidate chip initialization code Guenter Roeck
` (6 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Guenter Roeck @ 2024-08-30 1:05 UTC (permalink / raw)
To: Hardware Monitoring; +Cc: Tzung-Bi Shih, Guenter Roeck
Module tests show various overflow problems when writing limits
and other attributes.
in0_crit: Suspected overflow: [max=82, read 0, written 2147483648]
in0_lcrit: Suspected overflow: [max=82, read 0, written 2147483648]
in1_crit: Suspected overflow: [max=40959, read 0, written 2147483647]
in1_lcrit: Suspected overflow: [max=40959, read 0, written 2147483647]
power1_crit: Suspected overflow: [max=134218750, read 0, written 2147483648]
update_interval: Suspected overflow: [max=2253, read 2, written 2147483647]
Implement missing clamping on attribute write operations to avoid those
problems.
While at it, check in the probe function if the shunt resistor value
passed from devicetree is valid, and bail out if it isn't. Also limit
mutex use to the code calling ina2xx_set_shunt() since it isn't needed
when called from the probe function.
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Dropped unrelated cosmetic change
Added Reviewed-by: tag
drivers/hwmon/ina2xx.c | 32 ++++++++++++++++++++------------
1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index db6432523e59..2aea461b8c6d 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -193,10 +193,16 @@ static int ina226_reg_to_interval(u16 config)
* Return the new, shifted AVG field value of CONFIG register,
* to use with regmap_update_bits
*/
-static u16 ina226_interval_to_reg(int interval)
+static u16 ina226_interval_to_reg(unsigned long interval)
{
int avg, avg_bits;
+ /*
+ * The maximum supported interval is 1,024 * (2 * 8.244ms) ~= 16.8s.
+ * Clamp to 32 seconds before calculations to avoid overflows.
+ */
+ interval = clamp_val(interval, 0, 32000);
+
avg = DIV_ROUND_CLOSEST(interval * 1000,
INA226_TOTAL_CONV_TIME_DEFAULT);
avg_bits = find_closest(avg, ina226_avg_tab,
@@ -371,19 +377,22 @@ static int ina226_reg_to_alert(struct ina2xx_data *data, u32 mask, u16 regval)
* Turns alert limit values into register values.
* Opposite of the formula in ina2xx_get_value().
*/
-static u16 ina226_alert_to_reg(struct ina2xx_data *data, u32 mask, int val)
+static u16 ina226_alert_to_reg(struct ina2xx_data *data, u32 mask, unsigned long val)
{
switch (mask) {
case INA226_SHUNT_OVER_VOLTAGE_MASK:
case INA226_SHUNT_UNDER_VOLTAGE_MASK:
+ val = clamp_val(val, 0, SHRT_MAX * data->config->shunt_div);
val *= data->config->shunt_div;
- return clamp_val(val, SHRT_MIN, SHRT_MAX);
+ return clamp_val(val, 0, SHRT_MAX);
case INA226_BUS_OVER_VOLTAGE_MASK:
case INA226_BUS_UNDER_VOLTAGE_MASK:
+ val = clamp_val(val, 0, 200000);
val = (val * 1000) << data->config->bus_voltage_shift;
val = DIV_ROUND_CLOSEST(val, data->config->bus_voltage_lsb);
- return clamp_val(val, 0, SHRT_MAX);
+ return clamp_val(val, 0, USHRT_MAX);
case INA226_POWER_OVER_LIMIT_MASK:
+ val = clamp_val(val, 0, UINT_MAX - data->power_lsb_uW);
val = DIV_ROUND_CLOSEST(val, data->power_lsb_uW);
return clamp_val(val, 0, USHRT_MAX);
default:
@@ -490,19 +499,17 @@ static ssize_t ina226_alarm_show(struct device *dev,
* to shunt_voltage_lsb = 1 / shunt_div multiplied by 10^9 in order
* to keep the scale.
*/
-static int ina2xx_set_shunt(struct ina2xx_data *data, long val)
+static int ina2xx_set_shunt(struct ina2xx_data *data, unsigned long val)
{
unsigned int dividend = DIV_ROUND_CLOSEST(1000000000,
data->config->shunt_div);
- if (val <= 0 || val > dividend)
+ if (!val || val > dividend)
return -EINVAL;
- mutex_lock(&data->config_lock);
data->rshunt = val;
data->current_lsb_uA = DIV_ROUND_CLOSEST(dividend, val);
data->power_lsb_uW = data->config->power_lsb_factor *
data->current_lsb_uA;
- mutex_unlock(&data->config_lock);
return 0;
}
@@ -527,7 +534,9 @@ static ssize_t ina2xx_shunt_store(struct device *dev,
if (status < 0)
return status;
+ mutex_lock(&data->config_lock);
status = ina2xx_set_shunt(data, val);
+ mutex_unlock(&data->config_lock);
if (status < 0)
return status;
return count;
@@ -545,9 +554,6 @@ static ssize_t ina226_interval_store(struct device *dev,
if (status < 0)
return status;
- if (val > INT_MAX || val == 0)
- return -EINVAL;
-
status = regmap_update_bits(data->regmap, INA2XX_CONFIG,
INA226_AVG_RD_MASK,
ina226_interval_to_reg(val));
@@ -667,7 +673,9 @@ static int ina2xx_probe(struct i2c_client *client)
if (device_property_read_u32(dev, "shunt-resistor", &val) < 0)
val = INA2XX_RSHUNT_DEFAULT;
- ina2xx_set_shunt(data, val);
+ ret = ina2xx_set_shunt(data, val);
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "Invalid shunt resistor value\n");
data->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
if (IS_ERR(data->regmap)) {
--
2.45.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 08/14] hwmon: (ina2xx) Consolidate chip initialization code
2024-08-30 1:05 [PATCH v2 00/14] hwmon: (ina2xx) Cleanup and convert to use with_info API Guenter Roeck
` (6 preceding siblings ...)
2024-08-30 1:05 ` [PATCH v2 07/14] hwmon: (ina2xx) Fix various overflow issues Guenter Roeck
@ 2024-08-30 1:05 ` Guenter Roeck
2024-08-30 12:29 ` Tzung-Bi Shih
2024-08-30 1:05 ` [PATCH v2 09/14] hwmon: (ina2xx) Set alert latch Guenter Roeck
` (5 subsequent siblings)
13 siblings, 1 reply; 24+ messages in thread
From: Guenter Roeck @ 2024-08-30 1:05 UTC (permalink / raw)
To: Hardware Monitoring; +Cc: Tzung-Bi Shih, Guenter Roeck
Move all chip initialization code into a single function.
No functional change.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Fixed setting alert polarity
drivers/hwmon/ina2xx.c | 111 ++++++++++++++++-------------------------
1 file changed, 43 insertions(+), 68 deletions(-)
diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 2aea461b8c6d..a9e3b23445b4 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -67,9 +67,7 @@
#define INA226_READ_AVG(reg) FIELD_GET(INA226_AVG_RD_MASK, reg)
-#define INA226_ALERT_POLARITY_MASK BIT(1)
-#define INA226_ALERT_POL_LOW 0
-#define INA226_ALERT_POL_HIGH 1
+#define INA226_ALERT_POLARITY BIT(1)
/* bit number of alert functions in Mask/Enable Register */
#define INA226_SHUNT_OVER_VOLTAGE_MASK BIT(15)
@@ -141,6 +139,7 @@ struct ina2xx_config {
struct ina2xx_data {
const struct ina2xx_config *config;
+ enum ina2xx_ids chip;
long rshunt;
long current_lsb_uA;
@@ -211,39 +210,6 @@ static u16 ina226_interval_to_reg(unsigned long interval)
return FIELD_PREP(INA226_AVG_RD_MASK, avg_bits);
}
-static int ina2xx_set_alert_polarity(struct ina2xx_data *data,
- unsigned long val)
-{
- return regmap_update_bits(data->regmap, INA226_MASK_ENABLE,
- INA226_ALERT_POLARITY_MASK,
- FIELD_PREP(INA226_ALERT_POLARITY_MASK, val));
-}
-
-/*
- * Calibration register is set to the best value, which eliminates
- * truncation errors on calculating current register in hardware.
- * According to datasheet (eq. 3) the best values are 2048 for
- * ina226 and 4096 for ina219. They are hardcoded as calibration_value.
- */
-static int ina2xx_calibrate(struct ina2xx_data *data)
-{
- return regmap_write(data->regmap, INA2XX_CALIBRATION,
- data->config->calibration_value);
-}
-
-/*
- * Initialize the configuration and calibration registers.
- */
-static int ina2xx_init(struct ina2xx_data *data)
-{
- int ret = regmap_write(data->regmap, INA2XX_CONFIG,
- data->config->config_default);
- if (ret < 0)
- return ret;
-
- return ina2xx_calibrate(data);
-}
-
static int ina2xx_read_reg(struct device *dev, int reg, unsigned int *regval)
{
struct ina2xx_data *data = dev_get_drvdata(dev);
@@ -651,12 +617,48 @@ static const struct attribute_group ina226_group = {
.attrs = ina226_attrs,
};
+/*
+ * Initialize chip
+ */
+static int ina2xx_init(struct device *dev, struct ina2xx_data *data)
+{
+ struct regmap *regmap = data->regmap;
+ u32 shunt;
+ int ret;
+
+ if (device_property_read_u32(dev, "shunt-resistor", &shunt) < 0)
+ shunt = INA2XX_RSHUNT_DEFAULT;
+
+ ret = ina2xx_set_shunt(data, shunt);
+ if (ret < 0)
+ return ret;
+
+ ret = regmap_write(regmap, INA2XX_CONFIG, data->config->config_default);
+ if (ret < 0)
+ return ret;
+
+ if (data->chip == ina226) {
+ bool active_high = device_property_read_bool(dev, "ti,alert-polarity-active-high");
+
+ regmap_update_bits(regmap, INA226_MASK_ENABLE, INA226_ALERT_POLARITY,
+ FIELD_PREP(INA226_ALERT_POLARITY, active_high));
+ }
+
+ /*
+ * Calibration register is set to the best value, which eliminates
+ * truncation errors on calculating current register in hardware.
+ * According to datasheet (eq. 3) the best values are 2048 for
+ * ina226 and 4096 for ina219. They are hardcoded as calibration_value.
+ */
+ return regmap_write(regmap, INA2XX_CALIBRATION,
+ data->config->calibration_value);
+}
+
static int ina2xx_probe(struct i2c_client *client)
{
struct device *dev = &client->dev;
struct ina2xx_data *data;
struct device *hwmon_dev;
- u32 val;
int ret, group = 0;
enum ina2xx_ids chip;
@@ -668,15 +670,9 @@ static int ina2xx_probe(struct i2c_client *client)
/* set the device type */
data->config = &ina2xx_config[chip];
+ data->chip = chip;
mutex_init(&data->config_lock);
- if (device_property_read_u32(dev, "shunt-resistor", &val) < 0)
- val = INA2XX_RSHUNT_DEFAULT;
-
- ret = ina2xx_set_shunt(data, val);
- if (ret < 0)
- return dev_err_probe(dev, ret, "Invalid shunt resistor value\n");
-
data->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
if (IS_ERR(data->regmap)) {
dev_err(dev, "failed to allocate register map\n");
@@ -687,30 +683,9 @@ static int ina2xx_probe(struct i2c_client *client)
if (ret)
return dev_err_probe(dev, ret, "failed to enable vs regulator\n");
- if (chip == ina226) {
- if (device_property_read_bool(dev, "ti,alert-polarity-active-high")) {
- ret = ina2xx_set_alert_polarity(data,
- INA226_ALERT_POL_HIGH);
- if (ret < 0) {
- return dev_err_probe(dev, ret,
- "failed to set alert polarity active high\n");
- }
- } else {
- /* Set default value i.e active low */
- ret = ina2xx_set_alert_polarity(data,
- INA226_ALERT_POL_LOW);
- if (ret < 0) {
- return dev_err_probe(dev, ret,
- "failed to set alert polarity active low\n");
- }
- }
- }
-
- ret = ina2xx_init(data);
- if (ret < 0) {
- dev_err(dev, "error configuring the device: %d\n", ret);
- return -ENODEV;
- }
+ ret = ina2xx_init(dev, data);
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "failed to configure device\n");
data->groups[group++] = &ina2xx_group;
if (chip == ina226)
--
2.45.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 09/14] hwmon: (ina2xx) Set alert latch
2024-08-30 1:05 [PATCH v2 00/14] hwmon: (ina2xx) Cleanup and convert to use with_info API Guenter Roeck
` (7 preceding siblings ...)
2024-08-30 1:05 ` [PATCH v2 08/14] hwmon: (ina2xx) Consolidate chip initialization code Guenter Roeck
@ 2024-08-30 1:05 ` Guenter Roeck
2024-08-30 12:29 ` Tzung-Bi Shih
2024-08-30 1:05 ` [PATCH v2 10/14] hwmon: (ina2xx) Move ina2xx_get_value() Guenter Roeck
` (4 subsequent siblings)
13 siblings, 1 reply; 24+ messages in thread
From: Guenter Roeck @ 2024-08-30 1:05 UTC (permalink / raw)
To: Hardware Monitoring; +Cc: Tzung-Bi Shih, Guenter Roeck
Alerts should only be cleared after reported, not immediately after the
alert condition has been cleared. Set the latch enable bit to keep alerts
latched until the alert register has been read from the chip.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Moved patch after consolidation patch to keep it simple
Set latch bit once when initializing the chip and keep it active
drivers/hwmon/ina2xx.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index a9e3b23445b4..98338b7e7437 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -67,6 +67,7 @@
#define INA226_READ_AVG(reg) FIELD_GET(INA226_AVG_RD_MASK, reg)
+#define INA226_ALERT_LATCH_ENABLE BIT(0)
#define INA226_ALERT_POLARITY BIT(1)
/* bit number of alert functions in Mask/Enable Register */
@@ -640,8 +641,10 @@ static int ina2xx_init(struct device *dev, struct ina2xx_data *data)
if (data->chip == ina226) {
bool active_high = device_property_read_bool(dev, "ti,alert-polarity-active-high");
- regmap_update_bits(regmap, INA226_MASK_ENABLE, INA226_ALERT_POLARITY,
- FIELD_PREP(INA226_ALERT_POLARITY, active_high));
+ regmap_update_bits(regmap, INA226_MASK_ENABLE,
+ INA226_ALERT_LATCH_ENABLE | INA226_ALERT_POLARITY,
+ INA226_ALERT_LATCH_ENABLE |
+ FIELD_PREP(INA226_ALERT_POLARITY, active_high));
}
/*
--
2.45.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 10/14] hwmon: (ina2xx) Move ina2xx_get_value()
2024-08-30 1:05 [PATCH v2 00/14] hwmon: (ina2xx) Cleanup and convert to use with_info API Guenter Roeck
` (8 preceding siblings ...)
2024-08-30 1:05 ` [PATCH v2 09/14] hwmon: (ina2xx) Set alert latch Guenter Roeck
@ 2024-08-30 1:05 ` Guenter Roeck
2024-08-30 1:05 ` [PATCH v2 11/14] hwmon: (ina2xx) Convert to use with_info hwmon API Guenter Roeck
` (3 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Guenter Roeck @ 2024-08-30 1:05 UTC (permalink / raw)
To: Hardware Monitoring; +Cc: Tzung-Bi Shih, Guenter Roeck
ina2xx_get_value() will be needed earlier in the next patch, so move it.
No functional change.
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Added Reviewed-by: tag
drivers/hwmon/ina2xx.c | 72 +++++++++++++++++++++---------------------
1 file changed, 36 insertions(+), 36 deletions(-)
diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 98338b7e7437..fe850ee06024 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -211,6 +211,42 @@ static u16 ina226_interval_to_reg(unsigned long interval)
return FIELD_PREP(INA226_AVG_RD_MASK, avg_bits);
}
+static int ina2xx_get_value(struct ina2xx_data *data, u8 reg,
+ unsigned int regval)
+{
+ int val;
+
+ switch (reg) {
+ case INA2XX_SHUNT_VOLTAGE:
+ /* signed register */
+ val = DIV_ROUND_CLOSEST((s16)regval, data->config->shunt_div);
+ break;
+ case INA2XX_BUS_VOLTAGE:
+ val = (regval >> data->config->bus_voltage_shift) *
+ data->config->bus_voltage_lsb;
+ val = DIV_ROUND_CLOSEST(val, 1000);
+ break;
+ case INA2XX_POWER:
+ val = regval * data->power_lsb_uW;
+ break;
+ case INA2XX_CURRENT:
+ /* signed register, result in mA */
+ val = (s16)regval * data->current_lsb_uA;
+ val = DIV_ROUND_CLOSEST(val, 1000);
+ break;
+ case INA2XX_CALIBRATION:
+ val = regval;
+ break;
+ default:
+ /* programmer goofed */
+ WARN_ON_ONCE(1);
+ val = 0;
+ break;
+ }
+
+ return val;
+}
+
static int ina2xx_read_reg(struct device *dev, int reg, unsigned int *regval)
{
struct ina2xx_data *data = dev_get_drvdata(dev);
@@ -264,42 +300,6 @@ static int ina2xx_read_reg(struct device *dev, int reg, unsigned int *regval)
return -ENODEV;
}
-static int ina2xx_get_value(struct ina2xx_data *data, u8 reg,
- unsigned int regval)
-{
- int val;
-
- switch (reg) {
- case INA2XX_SHUNT_VOLTAGE:
- /* signed register */
- val = DIV_ROUND_CLOSEST((s16)regval, data->config->shunt_div);
- break;
- case INA2XX_BUS_VOLTAGE:
- val = (regval >> data->config->bus_voltage_shift)
- * data->config->bus_voltage_lsb;
- val = DIV_ROUND_CLOSEST(val, 1000);
- break;
- case INA2XX_POWER:
- val = regval * data->power_lsb_uW;
- break;
- case INA2XX_CURRENT:
- /* signed register, result in mA */
- val = (s16)regval * data->current_lsb_uA;
- val = DIV_ROUND_CLOSEST(val, 1000);
- break;
- case INA2XX_CALIBRATION:
- val = regval;
- break;
- default:
- /* programmer goofed */
- WARN_ON_ONCE(1);
- val = 0;
- break;
- }
-
- return val;
-}
-
static ssize_t ina2xx_value_show(struct device *dev,
struct device_attribute *da, char *buf)
{
--
2.45.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 11/14] hwmon: (ina2xx) Convert to use with_info hwmon API
2024-08-30 1:05 [PATCH v2 00/14] hwmon: (ina2xx) Cleanup and convert to use with_info API Guenter Roeck
` (9 preceding siblings ...)
2024-08-30 1:05 ` [PATCH v2 10/14] hwmon: (ina2xx) Move ina2xx_get_value() Guenter Roeck
@ 2024-08-30 1:05 ` Guenter Roeck
2024-08-30 12:30 ` Tzung-Bi Shih
2024-08-30 1:05 ` [PATCH v2 12/14] hwmon: (ina2xx) Pass register to alert limit write functions Guenter Roeck
` (2 subsequent siblings)
13 siblings, 1 reply; 24+ messages in thread
From: Guenter Roeck @ 2024-08-30 1:05 UTC (permalink / raw)
To: Hardware Monitoring; +Cc: Tzung-Bi Shih, Guenter Roeck
Convert driver to use the with_info hardware monitoring API
to reduce its dependency on sysfs attribute functions.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: No change
drivers/hwmon/ina2xx.c | 495 ++++++++++++++++++++++++-----------------
1 file changed, 293 insertions(+), 202 deletions(-)
diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index fe850ee06024..339d41dfa10e 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -25,9 +25,9 @@
#include <linux/bitfield.h>
#include <linux/bits.h>
#include <linux/delay.h>
+#include <linux/device.h>
#include <linux/err.h>
#include <linux/hwmon.h>
-#include <linux/hwmon-sysfs.h>
#include <linux/i2c.h>
#include <linux/init.h>
#include <linux/kernel.h>
@@ -35,6 +35,7 @@
#include <linux/property.h>
#include <linux/regmap.h>
#include <linux/slab.h>
+#include <linux/sysfs.h>
#include <linux/util_macros.h>
/* common register definitions */
@@ -81,9 +82,6 @@
#define INA226_ALERT_CONFIG_MASK GENMASK(15, 10)
#define INA226_ALERT_FUNCTION_FLAG BIT(4)
-/* common attrs, ina226 attrs and NULL */
-#define INA2XX_MAX_ATTRIBUTE_GROUPS 3
-
/*
* Both bus voltage and shunt voltage conversion times for ina226 are set
* to 0b0100 on POR, which translates to 2200 microseconds in total.
@@ -147,8 +145,6 @@ struct ina2xx_data {
long power_lsb_uW;
struct mutex config_lock;
struct regmap *regmap;
-
- const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
};
static const struct ina2xx_config ina2xx_config[] = {
@@ -193,7 +189,7 @@ static int ina226_reg_to_interval(u16 config)
* Return the new, shifted AVG field value of CONFIG register,
* to use with regmap_update_bits
*/
-static u16 ina226_interval_to_reg(unsigned long interval)
+static u16 ina226_interval_to_reg(long interval)
{
int avg, avg_bits;
@@ -247,14 +243,19 @@ static int ina2xx_get_value(struct ina2xx_data *data, u8 reg,
return val;
}
-static int ina2xx_read_reg(struct device *dev, int reg, unsigned int *regval)
+/*
+ * Read and convert register value from chip. If the register value is 0,
+ * check if the chip has been power cycled or reset. If so, re-initialize it.
+ */
+static int ina2xx_read_init(struct device *dev, int reg, long *val)
{
struct ina2xx_data *data = dev_get_drvdata(dev);
struct regmap *regmap = data->regmap;
+ unsigned int regval;
int ret, retry;
for (retry = 5; retry; retry--) {
- ret = regmap_read(regmap, reg, regval);
+ ret = regmap_read(regmap, reg, ®val);
if (ret < 0)
return ret;
@@ -266,7 +267,7 @@ static int ina2xx_read_reg(struct device *dev, int reg, unsigned int *regval)
* We do that extra read of the calibration register if there
* is some hint of a chip reset.
*/
- if (*regval == 0) {
+ if (regval == 0) {
unsigned int cal;
ret = regmap_read_bypassed(regmap, INA2XX_CALIBRATION, &cal);
@@ -288,6 +289,7 @@ static int ina2xx_read_reg(struct device *dev, int reg, unsigned int *regval)
continue;
}
}
+ *val = ina2xx_get_value(data, reg, regval);
return 0;
}
@@ -300,46 +302,6 @@ static int ina2xx_read_reg(struct device *dev, int reg, unsigned int *regval)
return -ENODEV;
}
-static ssize_t ina2xx_value_show(struct device *dev,
- struct device_attribute *da, char *buf)
-{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
- struct ina2xx_data *data = dev_get_drvdata(dev);
- unsigned int regval;
-
- int err = ina2xx_read_reg(dev, attr->index, ®val);
-
- if (err < 0)
- return err;
-
- return sysfs_emit(buf, "%d\n", ina2xx_get_value(data, attr->index, regval));
-}
-
-static int ina226_reg_to_alert(struct ina2xx_data *data, u32 mask, u16 regval)
-{
- int reg;
-
- switch (mask) {
- case INA226_SHUNT_OVER_VOLTAGE_MASK:
- case INA226_SHUNT_UNDER_VOLTAGE_MASK:
- reg = INA2XX_SHUNT_VOLTAGE;
- break;
- case INA226_BUS_OVER_VOLTAGE_MASK:
- case INA226_BUS_UNDER_VOLTAGE_MASK:
- reg = INA2XX_BUS_VOLTAGE;
- break;
- case INA226_POWER_OVER_LIMIT_MASK:
- reg = INA2XX_POWER;
- break;
- default:
- /* programmer goofed */
- WARN_ON_ONCE(1);
- return 0;
- }
-
- return ina2xx_get_value(data, reg, regval);
-}
-
/*
* Turns alert limit values into register values.
* Opposite of the formula in ina2xx_get_value().
@@ -369,14 +331,10 @@ static u16 ina226_alert_to_reg(struct ina2xx_data *data, u32 mask, unsigned long
}
}
-static ssize_t ina226_alert_show(struct device *dev,
- struct device_attribute *da, char *buf)
+static int ina226_alert_limit_read(struct ina2xx_data *data, u32 mask, int reg, long *val)
{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
- struct ina2xx_data *data = dev_get_drvdata(dev);
struct regmap *regmap = data->regmap;
int regval;
- int val = 0;
int ret;
mutex_lock(&data->config_lock);
@@ -384,32 +342,26 @@ static ssize_t ina226_alert_show(struct device *dev,
if (ret)
goto abort;
- if (regval & attr->index) {
+ if (regval & mask) {
ret = regmap_read(regmap, INA226_ALERT_LIMIT, ®val);
if (ret)
goto abort;
- val = ina226_reg_to_alert(data, attr->index, regval);
+ *val = ina2xx_get_value(data, reg, regval);
+ } else {
+ *val = 0;
}
-
- ret = sysfs_emit(buf, "%d\n", val);
abort:
mutex_unlock(&data->config_lock);
return ret;
}
-static ssize_t ina226_alert_store(struct device *dev,
- struct device_attribute *da,
- const char *buf, size_t count)
+static int ina226_alert_limit_write(struct ina2xx_data *data, u32 mask, long val)
{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
- struct ina2xx_data *data = dev_get_drvdata(dev);
struct regmap *regmap = data->regmap;
- unsigned long val;
int ret;
- ret = kstrtoul(buf, 10, &val);
- if (ret < 0)
- return ret;
+ if (val < 0)
+ return -EINVAL;
/*
* Clear all alerts first to avoid accidentally triggering ALERT pin
@@ -423,43 +375,285 @@ static ssize_t ina226_alert_store(struct device *dev,
goto abort;
ret = regmap_write(regmap, INA226_ALERT_LIMIT,
- ina226_alert_to_reg(data, attr->index, val));
+ ina226_alert_to_reg(data, mask, val));
if (ret < 0)
goto abort;
- if (val != 0) {
+ if (val)
ret = regmap_update_bits(regmap, INA226_MASK_ENABLE,
- INA226_ALERT_CONFIG_MASK,
- attr->index);
- if (ret < 0)
- goto abort;
- }
-
- ret = count;
+ INA226_ALERT_CONFIG_MASK, mask);
abort:
mutex_unlock(&data->config_lock);
return ret;
}
-static ssize_t ina226_alarm_show(struct device *dev,
- struct device_attribute *da, char *buf)
+static int ina2xx_chip_read(struct device *dev, u32 attr, long *val)
{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
struct ina2xx_data *data = dev_get_drvdata(dev);
- unsigned int mask;
- int alarm = 0;
+ u32 regval;
int ret;
- ret = regmap_read_bypassed(data->regmap, INA226_MASK_ENABLE, &mask);
+ switch (attr) {
+ case hwmon_chip_update_interval:
+ ret = regmap_read(data->regmap, INA2XX_CONFIG, ®val);
+ if (ret)
+ return ret;
+
+ *val = ina226_reg_to_interval(regval);
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+ return 0;
+}
+
+static int ina226_alert_read(struct regmap *regmap, u32 mask, long *val)
+{
+ unsigned int regval;
+ int ret;
+
+ ret = regmap_read_bypassed(regmap, INA226_MASK_ENABLE, ®val);
if (ret)
return ret;
- alarm = (mask & attr->index) &&
- (mask & INA226_ALERT_FUNCTION_FLAG);
+ *val = (regval & mask) && (regval & INA226_ALERT_FUNCTION_FLAG);
- return sysfs_emit(buf, "%d\n", alarm);
+ return 0;
}
+static int ina2xx_in_read(struct device *dev, u32 attr, int channel, long *val)
+{
+ int voltage_reg = channel ? INA2XX_BUS_VOLTAGE : INA2XX_SHUNT_VOLTAGE;
+ u32 under_voltage_mask = channel ? INA226_BUS_UNDER_VOLTAGE_MASK
+ : INA226_SHUNT_UNDER_VOLTAGE_MASK;
+ u32 over_voltage_mask = channel ? INA226_BUS_OVER_VOLTAGE_MASK
+ : INA226_SHUNT_OVER_VOLTAGE_MASK;
+ struct ina2xx_data *data = dev_get_drvdata(dev);
+ struct regmap *regmap = data->regmap;
+ unsigned int regval;
+ int ret;
+
+ switch (attr) {
+ case hwmon_in_input:
+ ret = regmap_read(regmap, voltage_reg, ®val);
+ if (ret)
+ return ret;
+ *val = ina2xx_get_value(data, voltage_reg, regval);
+ break;
+ case hwmon_in_lcrit:
+ return ina226_alert_limit_read(data, under_voltage_mask,
+ voltage_reg, val);
+ case hwmon_in_crit:
+ return ina226_alert_limit_read(data, over_voltage_mask,
+ voltage_reg, val);
+ case hwmon_in_lcrit_alarm:
+ return ina226_alert_read(regmap, under_voltage_mask, val);
+ case hwmon_in_crit_alarm:
+ return ina226_alert_read(regmap, over_voltage_mask, val);
+ default:
+ return -EOPNOTSUPP;
+ }
+ return 0;
+}
+
+static int ina2xx_power_read(struct device *dev, u32 attr, long *val)
+{
+ struct ina2xx_data *data = dev_get_drvdata(dev);
+
+ switch (attr) {
+ case hwmon_power_input:
+ return ina2xx_read_init(dev, INA2XX_POWER, val);
+ case hwmon_power_crit:
+ return ina226_alert_limit_read(data, INA226_POWER_OVER_LIMIT_MASK,
+ INA2XX_POWER, val);
+ case hwmon_power_crit_alarm:
+ return ina226_alert_read(data->regmap, INA226_POWER_OVER_LIMIT_MASK, val);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static int ina2xx_curr_read(struct device *dev, u32 attr, long *val)
+{
+ switch (attr) {
+ case hwmon_curr_input:
+ return ina2xx_read_init(dev, INA2XX_CURRENT, val);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static int ina2xx_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ switch (type) {
+ case hwmon_chip:
+ return ina2xx_chip_read(dev, attr, val);
+ case hwmon_in:
+ return ina2xx_in_read(dev, attr, channel, val);
+ case hwmon_power:
+ return ina2xx_power_read(dev, attr, val);
+ case hwmon_curr:
+ return ina2xx_curr_read(dev, attr, val);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static int ina2xx_chip_write(struct device *dev, u32 attr, long val)
+{
+ struct ina2xx_data *data = dev_get_drvdata(dev);
+
+ switch (attr) {
+ case hwmon_chip_update_interval:
+ return regmap_update_bits(data->regmap, INA2XX_CONFIG,
+ INA226_AVG_RD_MASK,
+ ina226_interval_to_reg(val));
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static int ina2xx_in_write(struct device *dev, u32 attr, int channel, long val)
+{
+ struct ina2xx_data *data = dev_get_drvdata(dev);
+
+ switch (attr) {
+ case hwmon_in_lcrit:
+ return ina226_alert_limit_write(data,
+ channel ? INA226_BUS_UNDER_VOLTAGE_MASK : INA226_SHUNT_UNDER_VOLTAGE_MASK,
+ val);
+ case hwmon_in_crit:
+ return ina226_alert_limit_write(data,
+ channel ? INA226_BUS_OVER_VOLTAGE_MASK : INA226_SHUNT_OVER_VOLTAGE_MASK,
+ val);
+ default:
+ return -EOPNOTSUPP;
+ }
+ return 0;
+}
+
+static int ina2xx_power_write(struct device *dev, u32 attr, long val)
+{
+ struct ina2xx_data *data = dev_get_drvdata(dev);
+
+ switch (attr) {
+ case hwmon_power_crit:
+ return ina226_alert_limit_write(data, INA226_POWER_OVER_LIMIT_MASK, val);
+ default:
+ return -EOPNOTSUPP;
+ }
+ return 0;
+}
+
+static int ina2xx_write(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long val)
+{
+ switch (type) {
+ case hwmon_chip:
+ return ina2xx_chip_write(dev, attr, val);
+ case hwmon_in:
+ return ina2xx_in_write(dev, attr, channel, val);
+ case hwmon_power:
+ return ina2xx_power_write(dev, attr, val);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static umode_t ina2xx_is_visible(const void *_data, enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ const struct ina2xx_data *data = _data;
+ enum ina2xx_ids chip = data->chip;
+
+ switch (type) {
+ case hwmon_in:
+ switch (attr) {
+ case hwmon_in_input:
+ return 0444;
+ case hwmon_in_lcrit:
+ case hwmon_in_crit:
+ if (chip == ina226)
+ return 0644;
+ break;
+ case hwmon_in_lcrit_alarm:
+ case hwmon_in_crit_alarm:
+ if (chip == ina226)
+ return 0444;
+ break;
+ default:
+ break;
+ }
+ break;
+ case hwmon_curr:
+ switch (attr) {
+ case hwmon_curr_input:
+ return 0444;
+ default:
+ break;
+ }
+ break;
+ case hwmon_power:
+ switch (attr) {
+ case hwmon_power_input:
+ return 0444;
+ case hwmon_power_crit:
+ if (chip == ina226)
+ return 0644;
+ break;
+ case hwmon_power_crit_alarm:
+ if (chip == ina226)
+ return 0444;
+ break;
+ default:
+ break;
+ }
+ break;
+ case hwmon_chip:
+ switch (attr) {
+ case hwmon_chip_update_interval:
+ if (chip == ina226)
+ return 0644;
+ break;
+ default:
+ break;
+ }
+ break;
+ default:
+ break;
+ }
+ return 0;
+}
+
+static const struct hwmon_channel_info * const ina2xx_info[] = {
+ HWMON_CHANNEL_INFO(chip,
+ HWMON_C_UPDATE_INTERVAL),
+ HWMON_CHANNEL_INFO(in,
+ HWMON_I_INPUT | HWMON_I_CRIT | HWMON_I_CRIT_ALARM |
+ HWMON_I_LCRIT | HWMON_I_LCRIT_ALARM,
+ HWMON_I_INPUT | HWMON_I_CRIT | HWMON_I_CRIT_ALARM |
+ HWMON_I_LCRIT | HWMON_I_LCRIT_ALARM
+ ),
+ HWMON_CHANNEL_INFO(curr, HWMON_C_INPUT),
+ HWMON_CHANNEL_INFO(power,
+ HWMON_P_INPUT | HWMON_P_CRIT | HWMON_P_CRIT_ALARM),
+ NULL
+};
+
+static const struct hwmon_ops ina2xx_hwmon_ops = {
+ .is_visible = ina2xx_is_visible,
+ .read = ina2xx_read,
+ .write = ina2xx_write,
+};
+
+static const struct hwmon_chip_info ina2xx_chip_info = {
+ .ops = &ina2xx_hwmon_ops,
+ .info = ina2xx_info,
+};
+
+/* shunt resistance */
+
/*
* In order to keep calibration register value fixed, the product
* of current_lsb and shunt_resistor should also be fixed and equal
@@ -481,21 +675,21 @@ static int ina2xx_set_shunt(struct ina2xx_data *data, unsigned long val)
return 0;
}
-static ssize_t ina2xx_shunt_show(struct device *dev,
- struct device_attribute *da, char *buf)
+static ssize_t shunt_resistor_show(struct device *dev,
+ struct device_attribute *da, char *buf)
{
struct ina2xx_data *data = dev_get_drvdata(dev);
return sysfs_emit(buf, "%li\n", data->rshunt);
}
-static ssize_t ina2xx_shunt_store(struct device *dev,
- struct device_attribute *da,
- const char *buf, size_t count)
+static ssize_t shunt_resistor_store(struct device *dev,
+ struct device_attribute *da,
+ const char *buf, size_t count)
{
+ struct ina2xx_data *data = dev_get_drvdata(dev);
unsigned long val;
int status;
- struct ina2xx_data *data = dev_get_drvdata(dev);
status = kstrtoul(buf, 10, &val);
if (status < 0)
@@ -509,114 +703,14 @@ static ssize_t ina2xx_shunt_store(struct device *dev,
return count;
}
-static ssize_t ina226_interval_store(struct device *dev,
- struct device_attribute *da,
- const char *buf, size_t count)
-{
- struct ina2xx_data *data = dev_get_drvdata(dev);
- unsigned long val;
- int status;
-
- status = kstrtoul(buf, 10, &val);
- if (status < 0)
- return status;
-
- status = regmap_update_bits(data->regmap, INA2XX_CONFIG,
- INA226_AVG_RD_MASK,
- ina226_interval_to_reg(val));
- if (status < 0)
- return status;
-
- return count;
-}
-
-static ssize_t ina226_interval_show(struct device *dev,
- struct device_attribute *da, char *buf)
-{
- struct ina2xx_data *data = dev_get_drvdata(dev);
- int status;
- unsigned int regval;
-
- status = regmap_read(data->regmap, INA2XX_CONFIG, ®val);
- if (status)
- return status;
-
- return sysfs_emit(buf, "%d\n", ina226_reg_to_interval(regval));
-}
-
-/* shunt voltage */
-static SENSOR_DEVICE_ATTR_RO(in0_input, ina2xx_value, INA2XX_SHUNT_VOLTAGE);
-/* shunt voltage over/under voltage alert setting and alarm */
-static SENSOR_DEVICE_ATTR_RW(in0_crit, ina226_alert,
- INA226_SHUNT_OVER_VOLTAGE_MASK);
-static SENSOR_DEVICE_ATTR_RW(in0_lcrit, ina226_alert,
- INA226_SHUNT_UNDER_VOLTAGE_MASK);
-static SENSOR_DEVICE_ATTR_RO(in0_crit_alarm, ina226_alarm,
- INA226_SHUNT_OVER_VOLTAGE_MASK);
-static SENSOR_DEVICE_ATTR_RO(in0_lcrit_alarm, ina226_alarm,
- INA226_SHUNT_UNDER_VOLTAGE_MASK);
-
-/* bus voltage */
-static SENSOR_DEVICE_ATTR_RO(in1_input, ina2xx_value, INA2XX_BUS_VOLTAGE);
-/* bus voltage over/under voltage alert setting and alarm */
-static SENSOR_DEVICE_ATTR_RW(in1_crit, ina226_alert,
- INA226_BUS_OVER_VOLTAGE_MASK);
-static SENSOR_DEVICE_ATTR_RW(in1_lcrit, ina226_alert,
- INA226_BUS_UNDER_VOLTAGE_MASK);
-static SENSOR_DEVICE_ATTR_RO(in1_crit_alarm, ina226_alarm,
- INA226_BUS_OVER_VOLTAGE_MASK);
-static SENSOR_DEVICE_ATTR_RO(in1_lcrit_alarm, ina226_alarm,
- INA226_BUS_UNDER_VOLTAGE_MASK);
-
-/* calculated current */
-static SENSOR_DEVICE_ATTR_RO(curr1_input, ina2xx_value, INA2XX_CURRENT);
-
-/* calculated power */
-static SENSOR_DEVICE_ATTR_RO(power1_input, ina2xx_value, INA2XX_POWER);
-/* over-limit power alert setting and alarm */
-static SENSOR_DEVICE_ATTR_RW(power1_crit, ina226_alert,
- INA226_POWER_OVER_LIMIT_MASK);
-static SENSOR_DEVICE_ATTR_RO(power1_crit_alarm, ina226_alarm,
- INA226_POWER_OVER_LIMIT_MASK);
-
-/* shunt resistance */
-static SENSOR_DEVICE_ATTR_RW(shunt_resistor, ina2xx_shunt, INA2XX_CALIBRATION);
-
-/* update interval (ina226 only) */
-static SENSOR_DEVICE_ATTR_RW(update_interval, ina226_interval, 0);
+static DEVICE_ATTR_RW(shunt_resistor);
/* pointers to created device attributes */
static struct attribute *ina2xx_attrs[] = {
- &sensor_dev_attr_in0_input.dev_attr.attr,
- &sensor_dev_attr_in1_input.dev_attr.attr,
- &sensor_dev_attr_curr1_input.dev_attr.attr,
- &sensor_dev_attr_power1_input.dev_attr.attr,
- &sensor_dev_attr_shunt_resistor.dev_attr.attr,
+ &dev_attr_shunt_resistor.attr,
NULL,
};
-
-static const struct attribute_group ina2xx_group = {
- .attrs = ina2xx_attrs,
-};
-
-static struct attribute *ina226_attrs[] = {
- &sensor_dev_attr_in0_crit.dev_attr.attr,
- &sensor_dev_attr_in0_lcrit.dev_attr.attr,
- &sensor_dev_attr_in0_crit_alarm.dev_attr.attr,
- &sensor_dev_attr_in0_lcrit_alarm.dev_attr.attr,
- &sensor_dev_attr_in1_crit.dev_attr.attr,
- &sensor_dev_attr_in1_lcrit.dev_attr.attr,
- &sensor_dev_attr_in1_crit_alarm.dev_attr.attr,
- &sensor_dev_attr_in1_lcrit_alarm.dev_attr.attr,
- &sensor_dev_attr_power1_crit.dev_attr.attr,
- &sensor_dev_attr_power1_crit_alarm.dev_attr.attr,
- &sensor_dev_attr_update_interval.dev_attr.attr,
- NULL,
-};
-
-static const struct attribute_group ina226_group = {
- .attrs = ina226_attrs,
-};
+ATTRIBUTE_GROUPS(ina2xx);
/*
* Initialize chip
@@ -662,8 +756,8 @@ static int ina2xx_probe(struct i2c_client *client)
struct device *dev = &client->dev;
struct ina2xx_data *data;
struct device *hwmon_dev;
- int ret, group = 0;
enum ina2xx_ids chip;
+ int ret;
chip = (uintptr_t)i2c_get_match_data(client);
@@ -690,12 +784,9 @@ static int ina2xx_probe(struct i2c_client *client)
if (ret < 0)
return dev_err_probe(dev, ret, "failed to configure device\n");
- data->groups[group++] = &ina2xx_group;
- if (chip == ina226)
- data->groups[group++] = &ina226_group;
-
- hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
- data, data->groups);
+ hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
+ data, &ina2xx_chip_info,
+ ina2xx_groups);
if (IS_ERR(hwmon_dev))
return PTR_ERR(hwmon_dev);
--
2.45.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 12/14] hwmon: (ina2xx) Pass register to alert limit write functions
2024-08-30 1:05 [PATCH v2 00/14] hwmon: (ina2xx) Cleanup and convert to use with_info API Guenter Roeck
` (10 preceding siblings ...)
2024-08-30 1:05 ` [PATCH v2 11/14] hwmon: (ina2xx) Convert to use with_info hwmon API Guenter Roeck
@ 2024-08-30 1:05 ` Guenter Roeck
2024-08-30 12:30 ` Tzung-Bi Shih
2024-08-30 1:05 ` [PATCH v2 13/14] hwmon: (ina2xx) Add support for current limits Guenter Roeck
2024-08-30 1:05 ` [PATCH v2 14/14] hwmon: (ina2xx) Use shunt voltage to calculate current Guenter Roeck
13 siblings, 1 reply; 24+ messages in thread
From: Guenter Roeck @ 2024-08-30 1:05 UTC (permalink / raw)
To: Hardware Monitoring; +Cc: Tzung-Bi Shih, Guenter Roeck
Pass the to-be-limited register to alert functions and use it to determine
conversion from limit to register value.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: New patch
drivers/hwmon/ina2xx.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 339d41dfa10e..1cd6fffb1495 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -306,21 +306,19 @@ static int ina2xx_read_init(struct device *dev, int reg, long *val)
* Turns alert limit values into register values.
* Opposite of the formula in ina2xx_get_value().
*/
-static u16 ina226_alert_to_reg(struct ina2xx_data *data, u32 mask, unsigned long val)
+static u16 ina226_alert_to_reg(struct ina2xx_data *data, int reg, unsigned long val)
{
- switch (mask) {
- case INA226_SHUNT_OVER_VOLTAGE_MASK:
- case INA226_SHUNT_UNDER_VOLTAGE_MASK:
+ switch (reg) {
+ case INA2XX_SHUNT_VOLTAGE:
val = clamp_val(val, 0, SHRT_MAX * data->config->shunt_div);
val *= data->config->shunt_div;
return clamp_val(val, 0, SHRT_MAX);
- case INA226_BUS_OVER_VOLTAGE_MASK:
- case INA226_BUS_UNDER_VOLTAGE_MASK:
+ case INA2XX_BUS_VOLTAGE:
val = clamp_val(val, 0, 200000);
val = (val * 1000) << data->config->bus_voltage_shift;
val = DIV_ROUND_CLOSEST(val, data->config->bus_voltage_lsb);
return clamp_val(val, 0, USHRT_MAX);
- case INA226_POWER_OVER_LIMIT_MASK:
+ case INA2XX_POWER:
val = clamp_val(val, 0, UINT_MAX - data->power_lsb_uW);
val = DIV_ROUND_CLOSEST(val, data->power_lsb_uW);
return clamp_val(val, 0, USHRT_MAX);
@@ -355,7 +353,7 @@ static int ina226_alert_limit_read(struct ina2xx_data *data, u32 mask, int reg,
return ret;
}
-static int ina226_alert_limit_write(struct ina2xx_data *data, u32 mask, long val)
+static int ina226_alert_limit_write(struct ina2xx_data *data, u32 mask, int reg, long val)
{
struct regmap *regmap = data->regmap;
int ret;
@@ -375,7 +373,7 @@ static int ina226_alert_limit_write(struct ina2xx_data *data, u32 mask, long val
goto abort;
ret = regmap_write(regmap, INA226_ALERT_LIMIT,
- ina226_alert_to_reg(data, mask, val));
+ ina226_alert_to_reg(data, reg, val));
if (ret < 0)
goto abort;
@@ -522,10 +520,12 @@ static int ina2xx_in_write(struct device *dev, u32 attr, int channel, long val)
case hwmon_in_lcrit:
return ina226_alert_limit_write(data,
channel ? INA226_BUS_UNDER_VOLTAGE_MASK : INA226_SHUNT_UNDER_VOLTAGE_MASK,
+ channel ? INA2XX_BUS_VOLTAGE : INA2XX_SHUNT_VOLTAGE,
val);
case hwmon_in_crit:
return ina226_alert_limit_write(data,
channel ? INA226_BUS_OVER_VOLTAGE_MASK : INA226_SHUNT_OVER_VOLTAGE_MASK,
+ channel ? INA2XX_BUS_VOLTAGE : INA2XX_SHUNT_VOLTAGE,
val);
default:
return -EOPNOTSUPP;
@@ -539,7 +539,8 @@ static int ina2xx_power_write(struct device *dev, u32 attr, long val)
switch (attr) {
case hwmon_power_crit:
- return ina226_alert_limit_write(data, INA226_POWER_OVER_LIMIT_MASK, val);
+ return ina226_alert_limit_write(data, INA226_POWER_OVER_LIMIT_MASK,
+ INA2XX_POWER, val);
default:
return -EOPNOTSUPP;
}
--
2.45.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 13/14] hwmon: (ina2xx) Add support for current limits
2024-08-30 1:05 [PATCH v2 00/14] hwmon: (ina2xx) Cleanup and convert to use with_info API Guenter Roeck
` (11 preceding siblings ...)
2024-08-30 1:05 ` [PATCH v2 12/14] hwmon: (ina2xx) Pass register to alert limit write functions Guenter Roeck
@ 2024-08-30 1:05 ` Guenter Roeck
2024-08-30 12:30 ` Tzung-Bi Shih
2024-08-30 1:05 ` [PATCH v2 14/14] hwmon: (ina2xx) Use shunt voltage to calculate current Guenter Roeck
13 siblings, 1 reply; 24+ messages in thread
From: Guenter Roeck @ 2024-08-30 1:05 UTC (permalink / raw)
To: Hardware Monitoring; +Cc: Tzung-Bi Shih, Guenter Roeck
While the chips supported by this driver do not directly support current
limits, they do support setting shunt voltage limits. The shunt voltage
divided by the shunt resistor value is the current. On top of that,
calibration values are set such that in the shunt voltage register and
the current register report the same values. That means we can report and
configure current limits based on shunt voltage limits, and we can do so
with much better accuracy than by setting shunt voltage limits.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: New patch
Documentation/hwmon/ina2xx.rst | 4 +++
drivers/hwmon/ina2xx.c | 61 ++++++++++++++++++++++++++++++++--
2 files changed, 63 insertions(+), 2 deletions(-)
diff --git a/Documentation/hwmon/ina2xx.rst b/Documentation/hwmon/ina2xx.rst
index 27d2e39bc8ac..7f1939b40f74 100644
--- a/Documentation/hwmon/ina2xx.rst
+++ b/Documentation/hwmon/ina2xx.rst
@@ -99,6 +99,10 @@ Sysfs entries for ina226, ina230 and ina231 only
------------------------------------------------
======================= ====================================================
+curr1_lcrit Critical low current
+curr1_crit Critical high current
+curr1_lcrit_alarm Current critical low alarm
+curr1_crit_alarm Current critical high alarm
in0_lcrit Critical low shunt voltage
in0_crit Critical high shunt voltage
in0_lcrit_alarm Shunt voltage critical low alarm
diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 1cd6fffb1495..10c8c475c634 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -306,7 +306,7 @@ static int ina2xx_read_init(struct device *dev, int reg, long *val)
* Turns alert limit values into register values.
* Opposite of the formula in ina2xx_get_value().
*/
-static u16 ina226_alert_to_reg(struct ina2xx_data *data, int reg, unsigned long val)
+static u16 ina226_alert_to_reg(struct ina2xx_data *data, int reg, long val)
{
switch (reg) {
case INA2XX_SHUNT_VOLTAGE:
@@ -322,6 +322,11 @@ static u16 ina226_alert_to_reg(struct ina2xx_data *data, int reg, unsigned long
val = clamp_val(val, 0, UINT_MAX - data->power_lsb_uW);
val = DIV_ROUND_CLOSEST(val, data->power_lsb_uW);
return clamp_val(val, 0, USHRT_MAX);
+ case INA2XX_CURRENT:
+ val = clamp_val(val, INT_MIN / 1000, INT_MAX / 1000);
+ /* signed register, result in mA */
+ val = DIV_ROUND_CLOSEST(val * 1000, data->current_lsb_uA);
+ return clamp_val(val, SHRT_MIN, SHRT_MAX);
default:
/* programmer goofed */
WARN_ON_ONCE(1);
@@ -473,9 +478,31 @@ static int ina2xx_power_read(struct device *dev, u32 attr, long *val)
static int ina2xx_curr_read(struct device *dev, u32 attr, long *val)
{
+ struct ina2xx_data *data = dev_get_drvdata(dev);
+ struct regmap *regmap = data->regmap;
+
+ /*
+ * While the chips supported by this driver do not directly support
+ * current limits, they do support setting shunt voltage limits.
+ * The shunt voltage divided by the shunt resistor value is the current.
+ * On top of that, calibration values are set such that in the shunt
+ * voltage register and the current register report the same values.
+ * That means we can report and configure current limits based on shunt
+ * voltage limits.
+ */
switch (attr) {
case hwmon_curr_input:
return ina2xx_read_init(dev, INA2XX_CURRENT, val);
+ case hwmon_curr_lcrit:
+ return ina226_alert_limit_read(data, INA226_SHUNT_UNDER_VOLTAGE_MASK,
+ INA2XX_CURRENT, val);
+ case hwmon_curr_crit:
+ return ina226_alert_limit_read(data, INA226_SHUNT_OVER_VOLTAGE_MASK,
+ INA2XX_CURRENT, val);
+ case hwmon_curr_lcrit_alarm:
+ return ina226_alert_read(regmap, INA226_SHUNT_UNDER_VOLTAGE_MASK, val);
+ case hwmon_curr_crit_alarm:
+ return ina226_alert_read(regmap, INA226_SHUNT_OVER_VOLTAGE_MASK, val);
default:
return -EOPNOTSUPP;
}
@@ -547,6 +574,23 @@ static int ina2xx_power_write(struct device *dev, u32 attr, long val)
return 0;
}
+static int ina2xx_curr_write(struct device *dev, u32 attr, long val)
+{
+ struct ina2xx_data *data = dev_get_drvdata(dev);
+
+ switch (attr) {
+ case hwmon_curr_lcrit:
+ return ina226_alert_limit_write(data, INA226_SHUNT_UNDER_VOLTAGE_MASK,
+ INA2XX_CURRENT, val);
+ case hwmon_curr_crit:
+ return ina226_alert_limit_write(data, INA226_SHUNT_OVER_VOLTAGE_MASK,
+ INA2XX_CURRENT, val);
+ default:
+ return -EOPNOTSUPP;
+ }
+ return 0;
+}
+
static int ina2xx_write(struct device *dev, enum hwmon_sensor_types type,
u32 attr, int channel, long val)
{
@@ -557,6 +601,8 @@ static int ina2xx_write(struct device *dev, enum hwmon_sensor_types type,
return ina2xx_in_write(dev, attr, channel, val);
case hwmon_power:
return ina2xx_power_write(dev, attr, val);
+ case hwmon_curr:
+ return ina2xx_curr_write(dev, attr, val);
default:
return -EOPNOTSUPP;
}
@@ -591,6 +637,16 @@ static umode_t ina2xx_is_visible(const void *_data, enum hwmon_sensor_types type
switch (attr) {
case hwmon_curr_input:
return 0444;
+ case hwmon_curr_lcrit:
+ case hwmon_curr_crit:
+ if (chip == ina226)
+ return 0644;
+ break;
+ case hwmon_curr_lcrit_alarm:
+ case hwmon_curr_crit_alarm:
+ if (chip == ina226)
+ return 0444;
+ break;
default:
break;
}
@@ -636,7 +692,8 @@ static const struct hwmon_channel_info * const ina2xx_info[] = {
HWMON_I_INPUT | HWMON_I_CRIT | HWMON_I_CRIT_ALARM |
HWMON_I_LCRIT | HWMON_I_LCRIT_ALARM
),
- HWMON_CHANNEL_INFO(curr, HWMON_C_INPUT),
+ 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),
NULL
--
2.45.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 14/14] hwmon: (ina2xx) Use shunt voltage to calculate current
2024-08-30 1:05 [PATCH v2 00/14] hwmon: (ina2xx) Cleanup and convert to use with_info API Guenter Roeck
` (12 preceding siblings ...)
2024-08-30 1:05 ` [PATCH v2 13/14] hwmon: (ina2xx) Add support for current limits Guenter Roeck
@ 2024-08-30 1:05 ` Guenter Roeck
2024-08-30 12:30 ` Tzung-Bi Shih
13 siblings, 1 reply; 24+ messages in thread
From: Guenter Roeck @ 2024-08-30 1:05 UTC (permalink / raw)
To: Hardware Monitoring; +Cc: Tzung-Bi Shih, Guenter Roeck
Since the shunt voltage and the current register report the same values
when the chip is calibrated, we can calculate the current directly
from the shunt voltage without relying on chip calibration.
With this change, the current register is no longer accessed. Its
register address is only used to indicate if reading or writing
current or shunt voltage is desired when accessing registers.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: New patch
drivers/hwmon/ina2xx.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 10c8c475c634..f0fa6d073627 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -480,6 +480,8 @@ static int ina2xx_curr_read(struct device *dev, u32 attr, long *val)
{
struct ina2xx_data *data = dev_get_drvdata(dev);
struct regmap *regmap = data->regmap;
+ unsigned int regval;
+ int ret;
/*
* While the chips supported by this driver do not directly support
@@ -492,7 +494,17 @@ static int ina2xx_curr_read(struct device *dev, u32 attr, long *val)
*/
switch (attr) {
case hwmon_curr_input:
- return ina2xx_read_init(dev, INA2XX_CURRENT, val);
+ /*
+ * Since the shunt voltage and the current register report the
+ * same values when the chip is calibrated, we can calculate
+ * the current directly from the shunt voltage without relying
+ * on chip calibration.
+ */
+ ret = regmap_read(regmap, INA2XX_SHUNT_VOLTAGE, ®val);
+ if (ret)
+ return ret;
+ *val = ina2xx_get_value(data, INA2XX_CURRENT, regval);
+ return 0;
case hwmon_curr_lcrit:
return ina226_alert_limit_read(data, INA226_SHUNT_UNDER_VOLTAGE_MASK,
INA2XX_CURRENT, val);
--
2.45.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 04/14] hwmon: (ina2xx) Mark regmap_config as const
2024-08-30 1:05 ` [PATCH v2 04/14] hwmon: (ina2xx) Mark regmap_config as const Guenter Roeck
@ 2024-08-30 12:28 ` Tzung-Bi Shih
0 siblings, 0 replies; 24+ messages in thread
From: Tzung-Bi Shih @ 2024-08-30 12:28 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Hardware Monitoring
On Thu, Aug 29, 2024 at 06:05:44PM -0700, Guenter Roeck wrote:
> Recent versions of checkpatch complain that struct regmap_config
> should be declared as const.
>
> WARNING: struct regmap_config should normally be const
>
> Doing so reveals a potential problem in the driver: If both supported
> chips are present in a single system, the maximum number of registers
> may race when devices are instantiated since max_registers is updated
> in the probe function. Solve the problem by setting .max_registers to the
> maximum register address of all supported chips. This does not make a
> practical difference while fixing the potential race condition and reducing
> code complexity.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 06/14] hwmon: (ina2xx) Re-initialize chip using regmap functions
2024-08-30 1:05 ` [PATCH v2 06/14] hwmon: (ina2xx) Re-initialize chip using regmap functions Guenter Roeck
@ 2024-08-30 12:29 ` Tzung-Bi Shih
0 siblings, 0 replies; 24+ messages in thread
From: Tzung-Bi Shih @ 2024-08-30 12:29 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Hardware Monitoring
On Thu, Aug 29, 2024 at 06:05:46PM -0700, Guenter Roeck wrote:
> If it is necessary to re-initialize the chip, for example because
> it has been power cycled, use regmap functions to update register
> contents. This ensures that all registers, including the configuration
> register and alert registers, are updated to previously configured
> values without having to locally cache everything.
>
> [...]
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 08/14] hwmon: (ina2xx) Consolidate chip initialization code
2024-08-30 1:05 ` [PATCH v2 08/14] hwmon: (ina2xx) Consolidate chip initialization code Guenter Roeck
@ 2024-08-30 12:29 ` Tzung-Bi Shih
0 siblings, 0 replies; 24+ messages in thread
From: Tzung-Bi Shih @ 2024-08-30 12:29 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Hardware Monitoring
On Thu, Aug 29, 2024 at 06:05:48PM -0700, Guenter Roeck wrote:
> Move all chip initialization code into a single function.
>
> No functional change.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 09/14] hwmon: (ina2xx) Set alert latch
2024-08-30 1:05 ` [PATCH v2 09/14] hwmon: (ina2xx) Set alert latch Guenter Roeck
@ 2024-08-30 12:29 ` Tzung-Bi Shih
0 siblings, 0 replies; 24+ messages in thread
From: Tzung-Bi Shih @ 2024-08-30 12:29 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Hardware Monitoring
On Thu, Aug 29, 2024 at 06:05:49PM -0700, Guenter Roeck wrote:
> Alerts should only be cleared after reported, not immediately after the
> alert condition has been cleared. Set the latch enable bit to keep alerts
> latched until the alert register has been read from the chip.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 11/14] hwmon: (ina2xx) Convert to use with_info hwmon API
2024-08-30 1:05 ` [PATCH v2 11/14] hwmon: (ina2xx) Convert to use with_info hwmon API Guenter Roeck
@ 2024-08-30 12:30 ` Tzung-Bi Shih
2024-08-30 15:32 ` Guenter Roeck
0 siblings, 1 reply; 24+ messages in thread
From: Tzung-Bi Shih @ 2024-08-30 12:30 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Hardware Monitoring
On Thu, Aug 29, 2024 at 06:05:51PM -0700, Guenter Roeck wrote:
> +static int ina2xx_chip_read(struct device *dev, u32 attr, long *val)
> {
> - struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> struct ina2xx_data *data = dev_get_drvdata(dev);
> - unsigned int mask;
> - int alarm = 0;
> + u32 regval;
> int ret;
>
> - ret = regmap_read_bypassed(data->regmap, INA226_MASK_ENABLE, &mask);
> + switch (attr) {
> + case hwmon_chip_update_interval:
> + ret = regmap_read(data->regmap, INA2XX_CONFIG, ®val);
> + if (ret)
> + return ret;
> +
> + *val = ina226_reg_to_interval(regval);
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> + return 0;
......(1)
> +static int ina2xx_power_read(struct device *dev, u32 attr, long *val)
> +{
> + struct ina2xx_data *data = dev_get_drvdata(dev);
> +
> + switch (attr) {
> + case hwmon_power_input:
> + return ina2xx_read_init(dev, INA2XX_POWER, val);
> + case hwmon_power_crit:
> + return ina226_alert_limit_read(data, INA226_POWER_OVER_LIMIT_MASK,
> + INA2XX_POWER, val);
> + case hwmon_power_crit_alarm:
> + return ina226_alert_read(data->regmap, INA226_POWER_OVER_LIMIT_MASK, val);
> + default:
> + return -EOPNOTSUPP;
> + }
......(2)
Just noticed a nit: there are some *_read() and *_write() functions mainly
contain a switch-case block. Some of them returns 0 at the end of the function
(1); some of them don't (2). (1) should be unreachable, however, I'm not sure
whether some checkers might complain about case (2).
In either cases, it would be great if make them consistent.
With that,
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 12/14] hwmon: (ina2xx) Pass register to alert limit write functions
2024-08-30 1:05 ` [PATCH v2 12/14] hwmon: (ina2xx) Pass register to alert limit write functions Guenter Roeck
@ 2024-08-30 12:30 ` Tzung-Bi Shih
0 siblings, 0 replies; 24+ messages in thread
From: Tzung-Bi Shih @ 2024-08-30 12:30 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Hardware Monitoring
On Thu, Aug 29, 2024 at 06:05:52PM -0700, Guenter Roeck wrote:
> Pass the to-be-limited register to alert functions and use it to determine
> conversion from limit to register value.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 13/14] hwmon: (ina2xx) Add support for current limits
2024-08-30 1:05 ` [PATCH v2 13/14] hwmon: (ina2xx) Add support for current limits Guenter Roeck
@ 2024-08-30 12:30 ` Tzung-Bi Shih
0 siblings, 0 replies; 24+ messages in thread
From: Tzung-Bi Shih @ 2024-08-30 12:30 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Hardware Monitoring
On Thu, Aug 29, 2024 at 06:05:53PM -0700, Guenter Roeck wrote:
> While the chips supported by this driver do not directly support current
> limits, they do support setting shunt voltage limits. The shunt voltage
> divided by the shunt resistor value is the current. On top of that,
> calibration values are set such that in the shunt voltage register and
> the current register report the same values. That means we can report and
> configure current limits based on shunt voltage limits, and we can do so
> with much better accuracy than by setting shunt voltage limits.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 14/14] hwmon: (ina2xx) Use shunt voltage to calculate current
2024-08-30 1:05 ` [PATCH v2 14/14] hwmon: (ina2xx) Use shunt voltage to calculate current Guenter Roeck
@ 2024-08-30 12:30 ` Tzung-Bi Shih
0 siblings, 0 replies; 24+ messages in thread
From: Tzung-Bi Shih @ 2024-08-30 12:30 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Hardware Monitoring
On Thu, Aug 29, 2024 at 06:05:54PM -0700, Guenter Roeck wrote:
> Since the shunt voltage and the current register report the same values
> when the chip is calibrated, we can calculate the current directly
> from the shunt voltage without relying on chip calibration.
>
> With this change, the current register is no longer accessed. Its
> register address is only used to indicate if reading or writing
> current or shunt voltage is desired when accessing registers.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 11/14] hwmon: (ina2xx) Convert to use with_info hwmon API
2024-08-30 12:30 ` Tzung-Bi Shih
@ 2024-08-30 15:32 ` Guenter Roeck
0 siblings, 0 replies; 24+ messages in thread
From: Guenter Roeck @ 2024-08-30 15:32 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: Hardware Monitoring
On 8/30/24 05:30, Tzung-Bi Shih wrote:
> On Thu, Aug 29, 2024 at 06:05:51PM -0700, Guenter Roeck wrote:
>> +static int ina2xx_chip_read(struct device *dev, u32 attr, long *val)
>> {
>> - struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
>> struct ina2xx_data *data = dev_get_drvdata(dev);
>> - unsigned int mask;
>> - int alarm = 0;
>> + u32 regval;
>> int ret;
>>
>> - ret = regmap_read_bypassed(data->regmap, INA226_MASK_ENABLE, &mask);
>> + switch (attr) {
>> + case hwmon_chip_update_interval:
>> + ret = regmap_read(data->regmap, INA2XX_CONFIG, ®val);
>> + if (ret)
>> + return ret;
>> +
>> + *val = ina226_reg_to_interval(regval);
>> + break;
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>> + return 0;
>
> ......(1)
>
>> +static int ina2xx_power_read(struct device *dev, u32 attr, long *val)
>> +{
>> + struct ina2xx_data *data = dev_get_drvdata(dev);
>> +
>> + switch (attr) {
>> + case hwmon_power_input:
>> + return ina2xx_read_init(dev, INA2XX_POWER, val);
>> + case hwmon_power_crit:
>> + return ina226_alert_limit_read(data, INA226_POWER_OVER_LIMIT_MASK,
>> + INA2XX_POWER, val);
>> + case hwmon_power_crit_alarm:
>> + return ina226_alert_read(data->regmap, INA226_POWER_OVER_LIMIT_MASK, val);
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>
> ......(2)
>
> Just noticed a nit: there are some *_read() and *_write() functions mainly
> contain a switch-case block. Some of them returns 0 at the end of the function
> (1); some of them don't (2). (1) should be unreachable, however, I'm not sure
> whether some checkers might complain about case (2).
>
Checkers are fine with it as long as each case statement returns. They actually
complain about unreachable code if there is a return statement at the end.
I've tried to return directly from a case statement if the return value from
a call is also the return value from the function. This is to avoid code such as
ret = func(...);
break;
...
return ret;
or, worse,
ret = func(...)
if (ret)
return ret;
break;
...
return 0;
and instead use
return func(...);
directly.
On the other side, if there is some code to execute after a function call
ret = func();
if (ret)
return ret;
do_something;
break;
...
return 0;
I tend to use return values and break statements. Maybe that doesn't always
work out. I'll keep it in mind for the future.
> In either cases, it would be great if make them consistent.
>
> With that,
> Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
Thanks a lot for this and all the other reviews!
Guenter
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2024-08-30 15:32 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-30 1:05 [PATCH v2 00/14] hwmon: (ina2xx) Cleanup and convert to use with_info API Guenter Roeck
2024-08-30 1:05 ` [PATCH v2 01/14] hwmon: (ina2xx) Reorder include files to alphabetic order Guenter Roeck
2024-08-30 1:05 ` [PATCH v2 02/14] hwmon: (ina2xx) Replace platform data with device properties Guenter Roeck
2024-08-30 1:05 ` [PATCH v2 03/14] hwmon: (ina2xx) Use bit operations Guenter Roeck
2024-08-30 1:05 ` [PATCH v2 04/14] hwmon: (ina2xx) Mark regmap_config as const Guenter Roeck
2024-08-30 12:28 ` Tzung-Bi Shih
2024-08-30 1:05 ` [PATCH v2 05/14] hwmon: (ina2xx) Use local regmap pointer if used more than once Guenter Roeck
2024-08-30 1:05 ` [PATCH v2 06/14] hwmon: (ina2xx) Re-initialize chip using regmap functions Guenter Roeck
2024-08-30 12:29 ` Tzung-Bi Shih
2024-08-30 1:05 ` [PATCH v2 07/14] hwmon: (ina2xx) Fix various overflow issues Guenter Roeck
2024-08-30 1:05 ` [PATCH v2 08/14] hwmon: (ina2xx) Consolidate chip initialization code Guenter Roeck
2024-08-30 12:29 ` Tzung-Bi Shih
2024-08-30 1:05 ` [PATCH v2 09/14] hwmon: (ina2xx) Set alert latch Guenter Roeck
2024-08-30 12:29 ` Tzung-Bi Shih
2024-08-30 1:05 ` [PATCH v2 10/14] hwmon: (ina2xx) Move ina2xx_get_value() Guenter Roeck
2024-08-30 1:05 ` [PATCH v2 11/14] hwmon: (ina2xx) Convert to use with_info hwmon API Guenter Roeck
2024-08-30 12:30 ` Tzung-Bi Shih
2024-08-30 15:32 ` Guenter Roeck
2024-08-30 1:05 ` [PATCH v2 12/14] hwmon: (ina2xx) Pass register to alert limit write functions Guenter Roeck
2024-08-30 12:30 ` Tzung-Bi Shih
2024-08-30 1:05 ` [PATCH v2 13/14] hwmon: (ina2xx) Add support for current limits Guenter Roeck
2024-08-30 12:30 ` Tzung-Bi Shih
2024-08-30 1:05 ` [PATCH v2 14/14] hwmon: (ina2xx) Use shunt voltage to calculate current Guenter Roeck
2024-08-30 12:30 ` Tzung-Bi Shih
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox