* [PATCH] hwmon: (ina3221) Add averaging mode support
@ 2019-03-12 22:04 Nicolin Chen
2019-03-12 22:37 ` Guenter Roeck
0 siblings, 1 reply; 3+ messages in thread
From: Nicolin Chen @ 2019-03-12 22:04 UTC (permalink / raw)
To: jdelvare, linux; +Cc: linux-hwmon, linux-kernel, corbet, linux-doc
The CONFIG register has a 3-bit averaging mode field for users
to setup the number of samples that are collected and averaged
together. This is very useful to filter noise from sensor data.
This patch adds an 'average' sysfs node, and updates wait time
calculation by taking this average value into account.
Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
Documentation/hwmon/ina3221 | 2 ++
drivers/hwmon/ina3221.c | 61 ++++++++++++++++++++++++++++++++++++-
2 files changed, 62 insertions(+), 1 deletion(-)
diff --git a/Documentation/hwmon/ina3221 b/Documentation/hwmon/ina3221
index 4b82cbfb551c..1e25be009d24 100644
--- a/Documentation/hwmon/ina3221
+++ b/Documentation/hwmon/ina3221
@@ -35,3 +35,5 @@ 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
+average Averaging mode. Supports the list of number of samples:
+ 1, 4, 16, 64, 128, 256, 512, 1024
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 3626b87a5fd2..259c192427ac 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -51,6 +51,9 @@
#define INA3221_CONFIG_VBUS_CT_SHIFT 6
#define INA3221_CONFIG_VBUS_CT_MASK GENMASK(8, 6)
#define INA3221_CONFIG_VBUS_CT(x) (((x) & GENMASK(8, 6)) >> 6)
+#define INA3221_CONFIG_AVG_SHIFT 9
+#define INA3221_CONFIG_AVG_MASK GENMASK(11, 9)
+#define INA3221_CONFIG_AVG(x) (((x) & GENMASK(11, 9)) >> 9)
#define INA3221_CONFIG_CHs_EN_MASK GENMASK(14, 12)
#define INA3221_CONFIG_CHx_EN(x) BIT(14 - (x))
@@ -135,17 +138,24 @@ static const u16 ina3221_conv_time[] = {
140, 204, 332, 588, 1100, 2116, 4156, 8244,
};
+/* Lookup table for averaging rates */
+static const int ina3221_avg[] = {
+ 1, 4, 16, 64, 128, 256, 512, 1024,
+};
+
static inline int ina3221_wait_for_data(struct ina3221_data *ina)
{
u32 channels = hweight16(ina->reg_config & INA3221_CONFIG_CHs_EN_MASK);
u32 vbus_ct_idx = INA3221_CONFIG_VBUS_CT(ina->reg_config);
u32 vsh_ct_idx = INA3221_CONFIG_VSH_CT(ina->reg_config);
+ u32 avg_idx = INA3221_CONFIG_AVG(ina->reg_config);
u32 vbus_ct = ina3221_conv_time[vbus_ct_idx];
u32 vsh_ct = ina3221_conv_time[vsh_ct_idx];
+ u32 avg = ina3221_avg[avg_idx];
u32 wait, cvrf;
/* Calculate total conversion time */
- wait = channels * (vbus_ct + vsh_ct);
+ wait = channels * (vbus_ct + vsh_ct) * avg;
/* Polling the CVRF bit to make sure read data is ready */
return regmap_field_read_poll_timeout(ina->fields[F_CVRF],
@@ -545,15 +555,64 @@ static ssize_t ina3221_shunt_store(struct device *dev,
return count;
}
+static ssize_t ina3221_avg_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct ina3221_data *ina = dev_get_drvdata(dev);
+ u32 avg_idx = INA3221_CONFIG_AVG(ina->reg_config);
+
+ return snprintf(buf, PAGE_SIZE, "%d\n", ina3221_avg[avg_idx]);
+}
+
+static ssize_t ina3221_avg_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct ina3221_data *ina = dev_get_drvdata(dev);
+ int ret, avg, i;
+
+ ret = kstrtoint(buf, 0, &avg);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < ARRAY_SIZE(ina3221_avg); i++)
+ if (ina3221_avg[i] == avg)
+ break;
+
+ if (i == ARRAY_SIZE(ina3221_avg))
+ return -EINVAL;
+
+ mutex_lock(&ina->lock);
+
+ ret = regmap_update_bits(ina->regmap, INA3221_CONFIG,
+ INA3221_CONFIG_AVG_MASK,
+ i << INA3221_CONFIG_AVG_SHIFT);
+ if (ret)
+ goto unlock;
+
+ /* Update reg_config accordingly */
+ ret = regmap_read(ina->regmap, INA3221_CONFIG, &ina->reg_config);
+ if (ret)
+ goto unlock;
+
+unlock:
+ mutex_unlock(&ina->lock);
+
+ return ret ? ret : count;
+}
+
/* shunt resistance */
static SENSOR_DEVICE_ATTR_RW(shunt1_resistor, ina3221_shunt, INA3221_CHANNEL1);
static SENSOR_DEVICE_ATTR_RW(shunt2_resistor, ina3221_shunt, INA3221_CHANNEL2);
static SENSOR_DEVICE_ATTR_RW(shunt3_resistor, ina3221_shunt, INA3221_CHANNEL3);
+/* averaging mode */
+static SENSOR_DEVICE_ATTR_RW(average, ina3221_avg, 0);
static struct attribute *ina3221_attrs[] = {
&sensor_dev_attr_shunt1_resistor.dev_attr.attr,
&sensor_dev_attr_shunt2_resistor.dev_attr.attr,
&sensor_dev_attr_shunt3_resistor.dev_attr.attr,
+ &sensor_dev_attr_average.dev_attr.attr,
NULL,
};
ATTRIBUTE_GROUPS(ina3221);
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] hwmon: (ina3221) Add averaging mode support
2019-03-12 22:04 [PATCH] hwmon: (ina3221) Add averaging mode support Nicolin Chen
@ 2019-03-12 22:37 ` Guenter Roeck
2019-03-12 22:52 ` Nicolin Chen
0 siblings, 1 reply; 3+ messages in thread
From: Guenter Roeck @ 2019-03-12 22:37 UTC (permalink / raw)
To: Nicolin Chen; +Cc: jdelvare, linux-hwmon, linux-kernel, corbet, linux-doc
On Tue, Mar 12, 2019 at 03:04:31PM -0700, Nicolin Chen wrote:
> The CONFIG register has a 3-bit averaging mode field for users
> to setup the number of samples that are collected and averaged
> together. This is very useful to filter noise from sensor data.
>
> This patch adds an 'average' sysfs node, and updates wait time
> calculation by taking this average value into account.
>
> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> ---
> Documentation/hwmon/ina3221 | 2 ++
> drivers/hwmon/ina3221.c | 61 ++++++++++++++++++++++++++++++++++++-
> 2 files changed, 62 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/hwmon/ina3221 b/Documentation/hwmon/ina3221
> index 4b82cbfb551c..1e25be009d24 100644
> --- a/Documentation/hwmon/ina3221
> +++ b/Documentation/hwmon/ina3221
> @@ -35,3 +35,5 @@ 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
> +average Averaging mode. Supports the list of number of samples:
> + 1, 4, 16, 64, 128, 256, 512, 1024
This is the number of samples, so I think "samples" would be a better
attribute name. This would also avoid confusion with other _average
attributes.
I'll need to check with other chips if this is the best approach and
name for the attribute, especially to see if it should be chip-wide
or per sensor.
> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> index 3626b87a5fd2..259c192427ac 100644
> --- a/drivers/hwmon/ina3221.c
> +++ b/drivers/hwmon/ina3221.c
> @@ -51,6 +51,9 @@
> #define INA3221_CONFIG_VBUS_CT_SHIFT 6
> #define INA3221_CONFIG_VBUS_CT_MASK GENMASK(8, 6)
> #define INA3221_CONFIG_VBUS_CT(x) (((x) & GENMASK(8, 6)) >> 6)
> +#define INA3221_CONFIG_AVG_SHIFT 9
> +#define INA3221_CONFIG_AVG_MASK GENMASK(11, 9)
> +#define INA3221_CONFIG_AVG(x) (((x) & GENMASK(11, 9)) >> 9)
> #define INA3221_CONFIG_CHs_EN_MASK GENMASK(14, 12)
> #define INA3221_CONFIG_CHx_EN(x) BIT(14 - (x))
>
> @@ -135,17 +138,24 @@ static const u16 ina3221_conv_time[] = {
> 140, 204, 332, 588, 1100, 2116, 4156, 8244,
> };
>
> +/* Lookup table for averaging rates */
> +static const int ina3221_avg[] = {
> + 1, 4, 16, 64, 128, 256, 512, 1024,
> +};
> +
> static inline int ina3221_wait_for_data(struct ina3221_data *ina)
> {
> u32 channels = hweight16(ina->reg_config & INA3221_CONFIG_CHs_EN_MASK);
> u32 vbus_ct_idx = INA3221_CONFIG_VBUS_CT(ina->reg_config);
> u32 vsh_ct_idx = INA3221_CONFIG_VSH_CT(ina->reg_config);
> + u32 avg_idx = INA3221_CONFIG_AVG(ina->reg_config);
> u32 vbus_ct = ina3221_conv_time[vbus_ct_idx];
> u32 vsh_ct = ina3221_conv_time[vsh_ct_idx];
> + u32 avg = ina3221_avg[avg_idx];
> u32 wait, cvrf;
>
> /* Calculate total conversion time */
> - wait = channels * (vbus_ct + vsh_ct);
> + wait = channels * (vbus_ct + vsh_ct) * avg;
>
> /* Polling the CVRF bit to make sure read data is ready */
> return regmap_field_read_poll_timeout(ina->fields[F_CVRF],
> @@ -545,15 +555,64 @@ static ssize_t ina3221_shunt_store(struct device *dev,
> return count;
> }
>
> +static ssize_t ina3221_avg_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct ina3221_data *ina = dev_get_drvdata(dev);
> + u32 avg_idx = INA3221_CONFIG_AVG(ina->reg_config);
> +
> + return snprintf(buf, PAGE_SIZE, "%d\n", ina3221_avg[avg_idx]);
> +}
> +
> +static ssize_t ina3221_avg_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct ina3221_data *ina = dev_get_drvdata(dev);
> + int ret, avg, i;
> +
> + ret = kstrtoint(buf, 0, &avg);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < ARRAY_SIZE(ina3221_avg); i++)
> + if (ina3221_avg[i] == avg)
> + break;
Please use find_closest().
> +
> + if (i == ARRAY_SIZE(ina3221_avg))
> + return -EINVAL;
> +
> + mutex_lock(&ina->lock);
> +
> + ret = regmap_update_bits(ina->regmap, INA3221_CONFIG,
> + INA3221_CONFIG_AVG_MASK,
> + i << INA3221_CONFIG_AVG_SHIFT);
> + if (ret)
> + goto unlock;
> +
> + /* Update reg_config accordingly */
> + ret = regmap_read(ina->regmap, INA3221_CONFIG, &ina->reg_config);
> + if (ret)
> + goto unlock;
> +
> +unlock:
> + mutex_unlock(&ina->lock);
> +
> + return ret ? ret : count;
> +}
> +
> /* shunt resistance */
> static SENSOR_DEVICE_ATTR_RW(shunt1_resistor, ina3221_shunt, INA3221_CHANNEL1);
> static SENSOR_DEVICE_ATTR_RW(shunt2_resistor, ina3221_shunt, INA3221_CHANNEL2);
> static SENSOR_DEVICE_ATTR_RW(shunt3_resistor, ina3221_shunt, INA3221_CHANNEL3);
> +/* averaging mode */
> +static SENSOR_DEVICE_ATTR_RW(average, ina3221_avg, 0);
>
> static struct attribute *ina3221_attrs[] = {
> &sensor_dev_attr_shunt1_resistor.dev_attr.attr,
> &sensor_dev_attr_shunt2_resistor.dev_attr.attr,
> &sensor_dev_attr_shunt3_resistor.dev_attr.attr,
> + &sensor_dev_attr_average.dev_attr.attr,
> NULL,
> };
> ATTRIBUTE_GROUPS(ina3221);
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] hwmon: (ina3221) Add averaging mode support
2019-03-12 22:37 ` Guenter Roeck
@ 2019-03-12 22:52 ` Nicolin Chen
0 siblings, 0 replies; 3+ messages in thread
From: Nicolin Chen @ 2019-03-12 22:52 UTC (permalink / raw)
To: Guenter Roeck; +Cc: jdelvare, linux-hwmon, linux-kernel, corbet, linux-doc
On Tue, Mar 12, 2019 at 03:37:59PM -0700, Guenter Roeck wrote:
> > +average Averaging mode. Supports the list of number of samples:
> > + 1, 4, 16, 64, 128, 256, 512, 1024
>
> This is the number of samples, so I think "samples" would be a better
> attribute name. This would also avoid confusion with other _average
> attributes.
>
> I'll need to check with other chips if this is the best approach and
> name for the attribute, especially to see if it should be chip-wide
> or per sensor.
Will wait for that then. Another thing is that the conversion times
are also configurable. And I plan to use the update_interval in the
ABI once this average is added. This means that average value will
be changed via this 'average' node (or other name), and conversion
times will be changed via the 'update_interval' node by following
the formula: update_interval = channels * (vbus_ct + vsh_ct) * avg
> > + for (i = 0; i < ARRAY_SIZE(ina3221_avg); i++)
> > + if (ina3221_avg[i] == avg)
> > + break;
>
> Please use find_closest().
OK. Thanks
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-03-12 22:52 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-12 22:04 [PATCH] hwmon: (ina3221) Add averaging mode support Nicolin Chen
2019-03-12 22:37 ` Guenter Roeck
2019-03-12 22:52 ` Nicolin Chen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).