linux-kernel - Re: [PATCH] ina3221: use CVRF only for single-shot conversion

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210604111120.GA1446736@roeck-us.net>
Date:   Fri, 4 Jun 2021 04:11:20 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Ninad Malwade <nmalwade@...dia.com>
Cc:     Jean Delvare <jdelvare@...e.com>, Bibek Basu <bbasu@...dia.com>,
        Nicolin Chen <nicolinc@...dia.com>,
        Rajkumar Kasirajan <rkasirajan@...dia.com>,
        linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ina3221: use CVRF only for single-shot conversion

On Fri, Jun 04, 2021 at 02:54:43PM +0800, Ninad Malwade wrote:
> As per current logic the wait time per conversion is arouns 430ms
> for 512 samples and around 860ms for 1024 samples for 3 channels
> considering 140us as the bus voltage and shunt voltage sampling
> conversion time.
> 
> This waiting time is a lot for the continuous mode and even for
> the single shot mode. For continuous mode when moving average is
> considered the waiting for CVRF bit is not required and the data
> from the previous conversion is sufficuent. As mentioned in the
> datasheet the conversion ready bit is provided to help coordinate
> single-shot conversions, we can restrict the use to single-shot
> mode only.
> 
> Also, the conversion time is for the averaged samples, the wait
> time for the polling can omit the number of samples consideration.
> 
Makes sense. Applied.

Thanks,
Guenter

> Signed-off-by: Ninad Malwade <nmalwade@...dia.com>
> ---
>  drivers/hwmon/ina3221.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> index c602583..58d3828 100644
> --- a/drivers/hwmon/ina3221.c
> +++ b/drivers/hwmon/ina3221.c
> @@ -196,13 +196,11 @@ static inline u32 ina3221_reg_to_interval_us(u16 config)
>  	u32 channels = hweight16(config & INA3221_CONFIG_CHs_EN_MASK);
>  	u32 vbus_ct_idx = INA3221_CONFIG_VBUS_CT(config);
>  	u32 vsh_ct_idx = INA3221_CONFIG_VSH_CT(config);
> -	u32 samples_idx = INA3221_CONFIG_AVG(config);
> -	u32 samples = ina3221_avg_samples[samples_idx];
>  	u32 vbus_ct = ina3221_conv_time[vbus_ct_idx];
>  	u32 vsh_ct = ina3221_conv_time[vsh_ct_idx];
>  
>  	/* Calculate total conversion time */
> -	return channels * (vbus_ct + vsh_ct) * samples;
> +	return channels * (vbus_ct + vsh_ct);
>  }
>  
>  static inline int ina3221_wait_for_data(struct ina3221_data *ina)
> @@ -288,13 +286,14 @@ static int ina3221_read_in(struct device *dev, u32 attr, int channel, long *val)
>  			return -ENODATA;
>  
>  		/* Write CONFIG register to trigger a single-shot measurement */
> -		if (ina->single_shot)
> +		if (ina->single_shot) {
>  			regmap_write(ina->regmap, INA3221_CONFIG,
>  				     ina->reg_config);
>  
> -		ret = ina3221_wait_for_data(ina);
> -		if (ret)
> -			return ret;
> +			ret = ina3221_wait_for_data(ina);
> +			if (ret)
> +				return ret;
> +		}
>  
>  		ret = ina3221_read_value(ina, reg, &regval);
>  		if (ret)
> @@ -344,13 +343,14 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
>  			return -ENODATA;
>  
>  		/* Write CONFIG register to trigger a single-shot measurement */
> -		if (ina->single_shot)
> +		if (ina->single_shot) {
>  			regmap_write(ina->regmap, INA3221_CONFIG,
>  				     ina->reg_config);
>  
> -		ret = ina3221_wait_for_data(ina);
> -		if (ret)
> -			return ret;
> +			ret = ina3221_wait_for_data(ina);
> +			if (ret)
> +				return ret;
> +		}
>  
>  		fallthrough;
>  	case hwmon_curr_crit: