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@xxxxxxxxx>
> ---
> 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
>
