Re: [PATCH] hwmon: (ina3221) Add averaging mode support

[Guenter Roeck <linux@roeck-us.net>]

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
>