| Message ID | 20221108050029.24576-1-nmalwade@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ina3221: correct the update_interval value | expand |
On Tue, Nov 08, 2022 at 01:00:29PM +0800, Ninad Malwade wrote: > As per the INA3221 datasheet the samples value should not be > considered while calculating the update_interval value. > Section 8.4.2.2 from datasheet says - "The conversion-time > settings, along with the programmable-averaging mode, enable > the INA3221 to optimize available timing requirements in a given > application. For example, if a system requires data to be read > every 2 ms with all three channels monitored, configure the INA3221 > with the conversion times for the shunt- and bus-voltage > measurements set to 332 μs" > > As per above only conversion time and number of channels are > required to set the update_interval value. Correcting the same in > the driver. > > Signed-off-by: Ninad Malwade <nmalwade@nvidia.com> > --- > Documentation/hwmon/ina3221.rst | 3 +-- > drivers/hwmon/ina3221.c | 4 +--- > 2 files changed, 2 insertions(+), 5 deletions(-) > > diff --git a/Documentation/hwmon/ina3221.rst b/Documentation/hwmon/ina3221.rst > index 8c12c54d2c24..a4f107d1e489 100644 > --- a/Documentation/hwmon/ina3221.rst > +++ b/Documentation/hwmon/ina3221.rst > @@ -61,10 +61,9 @@ samples Number of samples using in the averaging mode. > > update_interval Data conversion time in millisecond, following: > > - update_interval = C x S x (BC + SC) > + update_interval = C x (BC + SC) > > * C: number of enabled channels > - * S: number of samples > * BC: bus-voltage conversion time in millisecond > * SC: shunt-voltage conversion time in millisecond > > diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c > index 2a57f4b60c29..e3aa57e3b039 100644 > --- a/drivers/hwmon/ina3221.c > +++ b/drivers/hwmon/ina3221.c > @@ -183,11 +183,9 @@ static const int ina3221_avg_samples[] = { > static inline u32 ina3221_interval_ms_to_conv_time(u16 config, int interval) > { > u32 channels = hweight16(config & INA3221_CONFIG_CHs_EN_MASK); > - u32 samples_idx = INA3221_CONFIG_AVG(config); > - u32 samples = ina3221_avg_samples[samples_idx]; > > /* Bisect the result to Bus and Shunt conversion times */ > - return DIV_ROUND_CLOSEST(interval * 1000 / 2, channels * samples); > + return DIV_ROUND_CLOSEST(interval / 2, channels); Why do you drop the * 1000 multiplication factor? That seems to be there to account for the input "interval" being specified in ms whereas the values in ina3221_conv_time[] are in us. Isn't this new code going to give us values that are in the wrong unit? Thierry
On Tue, Nov 08, 2022 at 01:00:29PM +0800, Ninad Malwade wrote: > As per the INA3221 datasheet the samples value should not be > considered while calculating the update_interval value. > Section 8.4.2.2 from datasheet says - "The conversion-time > settings, along with the programmable-averaging mode, enable > the INA3221 to optimize available timing requirements in a given > application. For example, if a system requires data to be read > every 2 ms with all three channels monitored, configure the INA3221 > with the conversion times for the shunt- and bus-voltage > measurements set to 332 μs" > > As per above only conversion time and number of channels are > required to set the update_interval value. Correcting the same in > the driver. > > Signed-off-by: Ninad Malwade <nmalwade@nvidia.com> As a bug fix, this patch should come before any functional changes. > --- > Documentation/hwmon/ina3221.rst | 3 +-- > drivers/hwmon/ina3221.c | 4 +--- > 2 files changed, 2 insertions(+), 5 deletions(-) > > diff --git a/Documentation/hwmon/ina3221.rst b/Documentation/hwmon/ina3221.rst > index 8c12c54d2c24..a4f107d1e489 100644 > --- a/Documentation/hwmon/ina3221.rst > +++ b/Documentation/hwmon/ina3221.rst > @@ -61,10 +61,9 @@ samples Number of samples using in the averaging mode. > > update_interval Data conversion time in millisecond, following: > > - update_interval = C x S x (BC + SC) > + update_interval = C x (BC + SC) > > * C: number of enabled channels > - * S: number of samples > * BC: bus-voltage conversion time in millisecond > * SC: shunt-voltage conversion time in millisecond > > diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c > index 2a57f4b60c29..e3aa57e3b039 100644 > --- a/drivers/hwmon/ina3221.c > +++ b/drivers/hwmon/ina3221.c > @@ -183,11 +183,9 @@ static const int ina3221_avg_samples[] = { > static inline u32 ina3221_interval_ms_to_conv_time(u16 config, int interval) > { > u32 channels = hweight16(config & INA3221_CONFIG_CHs_EN_MASK); > - u32 samples_idx = INA3221_CONFIG_AVG(config); > - u32 samples = ina3221_avg_samples[samples_idx]; > > /* Bisect the result to Bus and Shunt conversion times */ > - return DIV_ROUND_CLOSEST(interval * 1000 / 2, channels * samples); > + return DIV_ROUND_CLOSEST(interval / 2, channels); Same question as Thierry: Why drop the multiplication ? Guenter > } > > /* Converting CONFIG register value to update_interval in usec */
diff --git a/Documentation/hwmon/ina3221.rst b/Documentation/hwmon/ina3221.rst index 8c12c54d2c24..a4f107d1e489 100644 --- a/Documentation/hwmon/ina3221.rst +++ b/Documentation/hwmon/ina3221.rst @@ -61,10 +61,9 @@ samples Number of samples using in the averaging mode. update_interval Data conversion time in millisecond, following: - update_interval = C x S x (BC + SC) + update_interval = C x (BC + SC) * C: number of enabled channels - * S: number of samples * BC: bus-voltage conversion time in millisecond * SC: shunt-voltage conversion time in millisecond diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c index 2a57f4b60c29..e3aa57e3b039 100644 --- a/drivers/hwmon/ina3221.c +++ b/drivers/hwmon/ina3221.c @@ -183,11 +183,9 @@ static const int ina3221_avg_samples[] = { static inline u32 ina3221_interval_ms_to_conv_time(u16 config, int interval) { u32 channels = hweight16(config & INA3221_CONFIG_CHs_EN_MASK); - u32 samples_idx = INA3221_CONFIG_AVG(config); - u32 samples = ina3221_avg_samples[samples_idx]; /* Bisect the result to Bus and Shunt conversion times */ - return DIV_ROUND_CLOSEST(interval * 1000 / 2, channels * samples); + return DIV_ROUND_CLOSEST(interval / 2, channels); } /* Converting CONFIG register value to update_interval in usec */
As per the INA3221 datasheet the samples value should not be considered while calculating the update_interval value. Section 8.4.2.2 from datasheet says - "The conversion-time settings, along with the programmable-averaging mode, enable the INA3221 to optimize available timing requirements in a given application. For example, if a system requires data to be read every 2 ms with all three channels monitored, configure the INA3221 with the conversion times for the shunt- and bus-voltage measurements set to 332 μs" As per above only conversion time and number of channels are required to set the update_interval value. Correcting the same in the driver. Signed-off-by: Ninad Malwade <nmalwade@nvidia.com> --- Documentation/hwmon/ina3221.rst | 3 +-- drivers/hwmon/ina3221.c | 4 +--- 2 files changed, 2 insertions(+), 5 deletions(-)