RE: [PATCH] ina3221: use CVRF only for single-shot conversion

[Ninad Malwade]

Thank You Guenter.

Regards,
-Ninad.

-----Original Message-----
From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
Sent: Friday, June 4, 2021 7:11 PM
To: Ninad Malwade <nmalwade@xxxxxxxxxx>
Cc: Jean Delvare <jdelvare@xxxxxxxx>; Bibek Basu <bbasu@xxxxxxxxxx>; Nicolin Chen <nicolinc@xxxxxxxxxx>; Rajkumar Kasirajan <rkasirajan@xxxxxxxxxx>; linux-hwmon@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
Subject: Re: [PATCH] ina3221: use CVRF only for single-shot conversion

External email: Use caution opening links or attachments


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@xxxxxxxxxx>
> ---
>  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: