* [PATCH] hwmon: (ina3221) Add voltage conversion time settings
@ 2019-04-16 23:55 Nicolin Chen
2019-04-17 13:46 ` Guenter Roeck
0 siblings, 1 reply; 7+ messages in thread
From: Nicolin Chen @ 2019-04-16 23:55 UTC (permalink / raw)
To: jdelvare, linux; +Cc: linux-hwmon, linux-kernel, corbet, linux-doc
The CONFIG register has two 3-bit fields for conversion time
settings of Bus-voltage and Shunt-voltage, respectively. The
conversion settings, along with averaging mode, allow users
to optimize available timing requirement.
This patch adds an 'update_interval' sysfs node through the
hwmon_chip_info of hwmon core. It reflects a total hardware
conversion time:
samples * channels * (Bus + Shunt conversion times)
Though INA3221 supports different conversion time setups for
Bus and Shunt voltages, this patch only adds the support of
a unified setting for both conversion times, by dividing the
conversion time into two equal values.
Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
Hi Guenter,
I am not quite sure if this update_interval is the best way to
implement the conversion time settings but I can't find another
common approach. This implementation certainly has drawbacks:
1) It can't configure Bus and Shunt conversion times separately
(Not crucial for me at this point as I set them equally)
2) Users need to calculate for the settings of conversion time
3) The ABI defines update_interval in msec while the hardware
and datasheet does in usec, and that generates rounding diff
4) The update_interval value would be spontaneously modified
everytime number of samples or number of enabled channels
gets changed. This might confuses users who tries to have
a fixed update_interval other than really merely setting
conversion time.
I see IIO subsystem have something like IIO_CHAN_INFO_INT_TIME
for conversion time setting exclusively. Do we have something
similar under hwmon?
Thanks
Documentation/hwmon/ina3221 | 9 +++++
drivers/hwmon/ina3221.c | 65 ++++++++++++++++++++++++++++++++-----
2 files changed, 65 insertions(+), 9 deletions(-)
diff --git a/Documentation/hwmon/ina3221 b/Documentation/hwmon/ina3221
index ed3f22769d4b..3b05170112f0 100644
--- a/Documentation/hwmon/ina3221
+++ b/Documentation/hwmon/ina3221
@@ -38,3 +38,12 @@ in[456]_input Shunt voltage(uV) for channels 1, 2, and 3 respectively
samples Number of samples using in the averaging mode.
Supports the list of number of samples:
1, 4, 16, 64, 128, 256, 512, 1024
+update_interval Data conversion time in millisecond, following:
+ update_interval = C x S 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
+ Affects both Bus- and Shunt-voltage conversion time.
+ Note that setting update_interval to 0ms sets both BC
+ and SC to 140 us (minimum conversion time).
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 74d39d212931..af4ab8ddddce 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -144,19 +144,37 @@ static const int ina3221_avg_samples[] = {
1, 4, 16, 64, 128, 256, 512, 1024,
};
-static inline int ina3221_wait_for_data(struct ina3221_data *ina)
+/* Converting update_interval in msec to conversion time in usec */
+static inline u32 ina3221_interval_ms_to_conv_time(u16 config, int interval)
{
- 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 samples_idx = INA3221_CONFIG_AVG(ina->reg_config);
+ 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);
+}
+
+/* Converting CONFIG register value to update_interval in usec */
+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];
- u32 wait, cvrf;
/* Calculate total conversion time */
- wait = channels * (vbus_ct + vsh_ct) * samples;
+ return channels * (vbus_ct + vsh_ct) * samples;
+}
+
+static inline int ina3221_wait_for_data(struct ina3221_data *ina)
+{
+ u32 wait, cvrf;
+
+ wait = ina3221_reg_to_interval_us(ina->reg_config);
/* Polling the CVRF bit to make sure read data is ready */
return regmap_field_read_poll_timeout(ina->fields[F_CVRF],
@@ -197,6 +215,11 @@ static int ina3221_read_chip(struct device *dev, u32 attr, long *val)
regval = INA3221_CONFIG_AVG(ina->reg_config);
*val = ina3221_avg_samples[regval];
return 0;
+ case hwmon_chip_update_interval:
+ /* Return in msec */
+ *val = ina3221_reg_to_interval_us(ina->reg_config);
+ *val = DIV_ROUND_CLOSEST(*val, 1000);
+ return 0;
default:
return -EOPNOTSUPP;
}
@@ -308,7 +331,7 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
static int ina3221_write_chip(struct device *dev, u32 attr, long val)
{
struct ina3221_data *ina = dev_get_drvdata(dev);
- int ret, idx;
+ int ret, idx, tmp;
switch (attr) {
case hwmon_chip_samples:
@@ -321,6 +344,28 @@ static int ina3221_write_chip(struct device *dev, u32 attr, long val)
if (ret)
return ret;
+ /* Update reg_config accordingly */
+ return regmap_read(ina->regmap, INA3221_CONFIG,
+ &ina->reg_config);
+ case hwmon_chip_update_interval:
+ tmp = ina3221_interval_ms_to_conv_time(ina->reg_config, val);
+ idx = find_closest(tmp, ina3221_conv_time,
+ ARRAY_SIZE(ina3221_conv_time));
+
+ /* Update Bus-voltage conversion time */
+ ret = regmap_update_bits(ina->regmap, INA3221_CONFIG,
+ INA3221_CONFIG_VBUS_CT_MASK,
+ idx << INA3221_CONFIG_VBUS_CT_SHIFT);
+ if (ret)
+ return ret;
+
+ /* Update Shunt-voltage conversion time */
+ ret = regmap_update_bits(ina->regmap, INA3221_CONFIG,
+ INA3221_CONFIG_VSH_CT_MASK,
+ idx << INA3221_CONFIG_VSH_CT_SHIFT);
+ if (ret)
+ return ret;
+
/* Update reg_config accordingly */
return regmap_read(ina->regmap, INA3221_CONFIG,
&ina->reg_config);
@@ -482,6 +527,7 @@ static umode_t ina3221_is_visible(const void *drvdata,
case hwmon_chip:
switch (attr) {
case hwmon_chip_samples:
+ case hwmon_chip_update_interval:
return 0644;
default:
return 0;
@@ -527,7 +573,8 @@ static umode_t ina3221_is_visible(const void *drvdata,
static const struct hwmon_channel_info *ina3221_info[] = {
HWMON_CHANNEL_INFO(chip,
- HWMON_C_SAMPLES),
+ HWMON_C_SAMPLES,
+ HWMON_C_UPDATE_INTERVAL),
HWMON_CHANNEL_INFO(in,
/* 0: dummy, skipped in is_visible */
HWMON_I_INPUT,
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] hwmon: (ina3221) Add voltage conversion time settings
2019-04-16 23:55 [PATCH] hwmon: (ina3221) Add voltage conversion time settings Nicolin Chen
@ 2019-04-17 13:46 ` Guenter Roeck
2019-04-17 14:04 ` Guenter Roeck
0 siblings, 1 reply; 7+ messages in thread
From: Guenter Roeck @ 2019-04-17 13:46 UTC (permalink / raw)
To: Nicolin Chen, jdelvare; +Cc: linux-hwmon, linux-kernel, corbet, linux-doc
On 4/16/19 4:55 PM, Nicolin Chen wrote:
> The CONFIG register has two 3-bit fields for conversion time
> settings of Bus-voltage and Shunt-voltage, respectively. The
> conversion settings, along with averaging mode, allow users
> to optimize available timing requirement.
>
> This patch adds an 'update_interval' sysfs node through the
> hwmon_chip_info of hwmon core. It reflects a total hardware
> conversion time:
> samples * channels * (Bus + Shunt conversion times)
>
> Though INA3221 supports different conversion time setups for
> Bus and Shunt voltages, this patch only adds the support of
> a unified setting for both conversion times, by dividing the
> conversion time into two equal values.
>
> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> ---
>
> Hi Guenter,
>
> I am not quite sure if this update_interval is the best way to
> implement the conversion time settings but I can't find another
> common approach. This implementation certainly has drawbacks:
> 1) It can't configure Bus and Shunt conversion times separately
> (Not crucial for me at this point as I set them equally)
> 2) Users need to calculate for the settings of conversion time
> 3) The ABI defines update_interval in msec while the hardware
> and datasheet does in usec, and that generates rounding diff
> 4) The update_interval value would be spontaneously modified
> everytime number of samples or number of enabled channels
> gets changed. This might confuses users who tries to have
> a fixed update_interval other than really merely setting
> conversion time.
>
> I see IIO subsystem have something like IIO_CHAN_INFO_INT_TIME
> for conversion time setting exclusively. Do we have something
> similar under hwmon?
>
No. I think what you have should be good enough for now.
Please see comments below.
Thanks,
Guenter
> Thanks
>
> Documentation/hwmon/ina3221 | 9 +++++
> drivers/hwmon/ina3221.c | 65 ++++++++++++++++++++++++++++++++-----
> 2 files changed, 65 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/hwmon/ina3221 b/Documentation/hwmon/ina3221
> index ed3f22769d4b..3b05170112f0 100644
> --- a/Documentation/hwmon/ina3221
> +++ b/Documentation/hwmon/ina3221
> @@ -38,3 +38,12 @@ in[456]_input Shunt voltage(uV) for channels 1, 2, and 3 respectively
> samples Number of samples using in the averaging mode.
> Supports the list of number of samples:
> 1, 4, 16, 64, 128, 256, 512, 1024
> +update_interval Data conversion time in millisecond, following:
> + update_interval = C x S 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
> + Affects both Bus- and Shunt-voltage conversion time.
> + Note that setting update_interval to 0ms sets both BC
> + and SC to 140 us (minimum conversion time).
> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> index 74d39d212931..af4ab8ddddce 100644
> --- a/drivers/hwmon/ina3221.c
> +++ b/drivers/hwmon/ina3221.c
> @@ -144,19 +144,37 @@ static const int ina3221_avg_samples[] = {
> 1, 4, 16, 64, 128, 256, 512, 1024,
> };
>
> -static inline int ina3221_wait_for_data(struct ina3221_data *ina)
> +/* Converting update_interval in msec to conversion time in usec */
> +static inline u32 ina3221_interval_ms_to_conv_time(u16 config, int interval)
> {
> - 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 samples_idx = INA3221_CONFIG_AVG(ina->reg_config);
> + 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);
> +}
> +
> +/* Converting CONFIG register value to update_interval in usec */
> +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];
> - u32 wait, cvrf;
>
> /* Calculate total conversion time */
> - wait = channels * (vbus_ct + vsh_ct) * samples;
> + return channels * (vbus_ct + vsh_ct) * samples;
> +}
> +
> +static inline int ina3221_wait_for_data(struct ina3221_data *ina)
> +{
> + u32 wait, cvrf;
> +
> + wait = ina3221_reg_to_interval_us(ina->reg_config);
>
> /* Polling the CVRF bit to make sure read data is ready */
> return regmap_field_read_poll_timeout(ina->fields[F_CVRF],
> @@ -197,6 +215,11 @@ static int ina3221_read_chip(struct device *dev, u32 attr, long *val)
> regval = INA3221_CONFIG_AVG(ina->reg_config);
> *val = ina3221_avg_samples[regval];
> return 0;
> + case hwmon_chip_update_interval:
> + /* Return in msec */
> + *val = ina3221_reg_to_interval_us(ina->reg_config);
> + *val = DIV_ROUND_CLOSEST(*val, 1000);
> + return 0;
> default:
> return -EOPNOTSUPP;
> }
> @@ -308,7 +331,7 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
> static int ina3221_write_chip(struct device *dev, u32 attr, long val)
> {
> struct ina3221_data *ina = dev_get_drvdata(dev);
> - int ret, idx;
> + int ret, idx, tmp;
>
> switch (attr) {
> case hwmon_chip_samples:
> @@ -321,6 +344,28 @@ static int ina3221_write_chip(struct device *dev, u32 attr, long val)
> if (ret)
> return ret;
>
> + /* Update reg_config accordingly */
> + return regmap_read(ina->regmap, INA3221_CONFIG,
> + &ina->reg_config);
> + case hwmon_chip_update_interval:
> + tmp = ina3221_interval_ms_to_conv_time(ina->reg_config, val);
> + idx = find_closest(tmp, ina3221_conv_time,
> + ARRAY_SIZE(ina3221_conv_time));
> +
> + /* Update Bus-voltage conversion time */
> + ret = regmap_update_bits(ina->regmap, INA3221_CONFIG,
> + INA3221_CONFIG_VBUS_CT_MASK,
> + idx << INA3221_CONFIG_VBUS_CT_SHIFT);
> + if (ret)
> + return ret;
> +
> + /* Update Shunt-voltage conversion time */
> + ret = regmap_update_bits(ina->regmap, INA3221_CONFIG,
> + INA3221_CONFIG_VSH_CT_MASK,
> + idx << INA3221_CONFIG_VSH_CT_SHIFT);
> + if (ret)
> + return ret;
It should be possible to update both conversion times with a single call,
since both calls touch only one register. Something like
ret = regmap_update_bits(ina->regmap, INA3221_CONFIG
INA3221_CONFIG_VBUS_CT_MASK | INA3221_CONFIG_VSH_CT_MASK,
(idx << INA3221_CONFIG_VBUS_CT_SHIFT) | (idx << INA3221_CONFIG_VSH_CT_SHIFT));
Granted, that is a bit long, but it saves an extra i2c write operation.
> +
> /* Update reg_config accordingly */
> return regmap_read(ina->regmap, INA3221_CONFIG,
> &ina->reg_config);
> @@ -482,6 +527,7 @@ static umode_t ina3221_is_visible(const void *drvdata,
> case hwmon_chip:
> switch (attr) {
> case hwmon_chip_samples:
> + case hwmon_chip_update_interval:
> return 0644;
> default:
> return 0;
> @@ -527,7 +573,8 @@ static umode_t ina3221_is_visible(const void *drvdata,
>
> static const struct hwmon_channel_info *ina3221_info[] = {
> HWMON_CHANNEL_INFO(chip,
> - HWMON_C_SAMPLES),
> + HWMON_C_SAMPLES,
> + HWMON_C_UPDATE_INTERVAL),
> HWMON_CHANNEL_INFO(in,
> /* 0: dummy, skipped in is_visible */
> HWMON_I_INPUT,
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hwmon: (ina3221) Add voltage conversion time settings
2019-04-17 13:46 ` Guenter Roeck
@ 2019-04-17 14:04 ` Guenter Roeck
2019-04-17 18:39 ` Nicolin Chen
0 siblings, 1 reply; 7+ messages in thread
From: Guenter Roeck @ 2019-04-17 14:04 UTC (permalink / raw)
To: Nicolin Chen, jdelvare; +Cc: linux-hwmon, linux-kernel, corbet, linux-doc
On 4/17/19 6:46 AM, Guenter Roeck wrote:
> On 4/16/19 4:55 PM, Nicolin Chen wrote:
>> The CONFIG register has two 3-bit fields for conversion time
>> settings of Bus-voltage and Shunt-voltage, respectively. The
>> conversion settings, along with averaging mode, allow users
>> to optimize available timing requirement.
>>
>> This patch adds an 'update_interval' sysfs node through the
>> hwmon_chip_info of hwmon core. It reflects a total hardware
>> conversion time:
>> samples * channels * (Bus + Shunt conversion times)
>>
>> Though INA3221 supports different conversion time setups for
>> Bus and Shunt voltages, this patch only adds the support of
>> a unified setting for both conversion times, by dividing the
>> conversion time into two equal values.
>>
>> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
>> ---
>>
>> Hi Guenter,
>>
>> I am not quite sure if this update_interval is the best way to
>> implement the conversion time settings but I can't find another
>> common approach. This implementation certainly has drawbacks:
>> 1) It can't configure Bus and Shunt conversion times separately
>> (Not crucial for me at this point as I set them equally)
>> 2) Users need to calculate for the settings of conversion time
>> 3) The ABI defines update_interval in msec while the hardware
>> and datasheet does in usec, and that generates rounding diff
>> 4) The update_interval value would be spontaneously modified
>> everytime number of samples or number of enabled channels
>> gets changed. This might confuses users who tries to have
>> a fixed update_interval other than really merely setting
>> conversion time.
>>
>> I see IIO subsystem have something like IIO_CHAN_INFO_INT_TIME
>> for conversion time setting exclusively. Do we have something
>> similar under hwmon?
>>
>
> No. I think what you have should be good enough for now.
> Please see comments below.
>
> Thanks,
> Guenter
>
>> Thanks
>>
>> Documentation/hwmon/ina3221 | 9 +++++
>> drivers/hwmon/ina3221.c | 65 ++++++++++++++++++++++++++++++++-----
>> 2 files changed, 65 insertions(+), 9 deletions(-)
>>
>> diff --git a/Documentation/hwmon/ina3221 b/Documentation/hwmon/ina3221
>> index ed3f22769d4b..3b05170112f0 100644
>> --- a/Documentation/hwmon/ina3221
>> +++ b/Documentation/hwmon/ina3221
>> @@ -38,3 +38,12 @@ in[456]_input Shunt voltage(uV) for channels 1, 2, and 3 respectively
>> samples Number of samples using in the averaging mode.
>> Supports the list of number of samples:
>> 1, 4, 16, 64, 128, 256, 512, 1024
>> +update_interval Data conversion time in millisecond, following:
>> + update_interval = C x S 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
>> + Affects both Bus- and Shunt-voltage conversion time.
>> + Note that setting update_interval to 0ms sets both BC
>> + and SC to 140 us (minimum conversion time).
>> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
>> index 74d39d212931..af4ab8ddddce 100644
>> --- a/drivers/hwmon/ina3221.c
>> +++ b/drivers/hwmon/ina3221.c
>> @@ -144,19 +144,37 @@ static const int ina3221_avg_samples[] = {
>> 1, 4, 16, 64, 128, 256, 512, 1024,
>> };
>> -static inline int ina3221_wait_for_data(struct ina3221_data *ina)
>> +/* Converting update_interval in msec to conversion time in usec */
>> +static inline u32 ina3221_interval_ms_to_conv_time(u16 config, int interval)
>> {
>> - 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 samples_idx = INA3221_CONFIG_AVG(ina->reg_config);
>> + 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);
>> +}
>> +
>> +/* Converting CONFIG register value to update_interval in usec */
>> +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];
>> - u32 wait, cvrf;
>> /* Calculate total conversion time */
>> - wait = channels * (vbus_ct + vsh_ct) * samples;
>> + return channels * (vbus_ct + vsh_ct) * samples;
>> +}
>> +
>> +static inline int ina3221_wait_for_data(struct ina3221_data *ina)
>> +{
>> + u32 wait, cvrf;
>> +
>> + wait = ina3221_reg_to_interval_us(ina->reg_config);
>> /* Polling the CVRF bit to make sure read data is ready */
>> return regmap_field_read_poll_timeout(ina->fields[F_CVRF],
>> @@ -197,6 +215,11 @@ static int ina3221_read_chip(struct device *dev, u32 attr, long *val)
>> regval = INA3221_CONFIG_AVG(ina->reg_config);
>> *val = ina3221_avg_samples[regval];
>> return 0;
>> + case hwmon_chip_update_interval:
>> + /* Return in msec */
>> + *val = ina3221_reg_to_interval_us(ina->reg_config);
>> + *val = DIV_ROUND_CLOSEST(*val, 1000);
>> + return 0;
>> default:
>> return -EOPNOTSUPP;
>> }
>> @@ -308,7 +331,7 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
>> static int ina3221_write_chip(struct device *dev, u32 attr, long val)
>> {
>> struct ina3221_data *ina = dev_get_drvdata(dev);
>> - int ret, idx;
>> + int ret, idx, tmp;
>> switch (attr) {
>> case hwmon_chip_samples:
>> @@ -321,6 +344,28 @@ static int ina3221_write_chip(struct device *dev, u32 attr, long val)
>> if (ret)
>> return ret;
>> + /* Update reg_config accordingly */
>> + return regmap_read(ina->regmap, INA3221_CONFIG,
>> + &ina->reg_config);
>> + case hwmon_chip_update_interval:
>> + tmp = ina3221_interval_ms_to_conv_time(ina->reg_config, val);
>> + idx = find_closest(tmp, ina3221_conv_time,
>> + ARRAY_SIZE(ina3221_conv_time));
>> +
>> + /* Update Bus-voltage conversion time */
>> + ret = regmap_update_bits(ina->regmap, INA3221_CONFIG,
>> + INA3221_CONFIG_VBUS_CT_MASK,
>> + idx << INA3221_CONFIG_VBUS_CT_SHIFT);
>> + if (ret)
>> + return ret;
>> +
>> + /* Update Shunt-voltage conversion time */
>> + ret = regmap_update_bits(ina->regmap, INA3221_CONFIG,
>> + INA3221_CONFIG_VSH_CT_MASK,
>> + idx << INA3221_CONFIG_VSH_CT_SHIFT);
>> + if (ret)
>> + return ret;
>
> It should be possible to update both conversion times with a single call,
> since both calls touch only one register. Something like
>
> ret = regmap_update_bits(ina->regmap, INA3221_CONFIG
> INA3221_CONFIG_VBUS_CT_MASK | INA3221_CONFIG_VSH_CT_MASK,
> (idx << INA3221_CONFIG_VBUS_CT_SHIFT) | (idx << INA3221_CONFIG_VSH_CT_SHIFT));
>
> Granted, that is a bit long, but it saves an extra i2c write operation.
>
Thinking about it ... does it even make sense to cache reg_config twice,
or would it be better to just update the local copy and use regmap_write()
to send it to the chip ?
Guenter
>> +
>> /* Update reg_config accordingly */
>> return regmap_read(ina->regmap, INA3221_CONFIG,
>> &ina->reg_config);
>> @@ -482,6 +527,7 @@ static umode_t ina3221_is_visible(const void *drvdata,
>> case hwmon_chip:
>> switch (attr) {
>> case hwmon_chip_samples:
>> + case hwmon_chip_update_interval:
>> return 0644;
>> default:
>> return 0;
>> @@ -527,7 +573,8 @@ static umode_t ina3221_is_visible(const void *drvdata,
>> static const struct hwmon_channel_info *ina3221_info[] = {
>> HWMON_CHANNEL_INFO(chip,
>> - HWMON_C_SAMPLES),
>> + HWMON_C_SAMPLES,
>> + HWMON_C_UPDATE_INTERVAL),
>> HWMON_CHANNEL_INFO(in,
>> /* 0: dummy, skipped in is_visible */
>> HWMON_I_INPUT,
>>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hwmon: (ina3221) Add voltage conversion time settings
2019-04-17 14:04 ` Guenter Roeck
@ 2019-04-17 18:39 ` Nicolin Chen
2019-04-17 19:48 ` Nicolin Chen
0 siblings, 1 reply; 7+ messages in thread
From: Nicolin Chen @ 2019-04-17 18:39 UTC (permalink / raw)
To: Guenter Roeck; +Cc: jdelvare, linux-hwmon, linux-kernel, corbet, linux-doc
On Wed, Apr 17, 2019 at 07:04:09AM -0700, Guenter Roeck wrote:
> > > I am not quite sure if this update_interval is the best way to
> > > implement the conversion time settings but I can't find another
> > > common approach. This implementation certainly has drawbacks:
> > > 1) It can't configure Bus and Shunt conversion times separately
> > > (Not crucial for me at this point as I set them equally)
> > > 2) Users need to calculate for the settings of conversion time
> > > 3) The ABI defines update_interval in msec while the hardware
> > > and datasheet does in usec, and that generates rounding diff
> > > 4) The update_interval value would be spontaneously modified
> > > everytime number of samples or number of enabled channels
> > > gets changed. This might confuses users who tries to have
> > > a fixed update_interval other than really merely setting
> > > conversion time.
> > >
> > > I see IIO subsystem have something like IIO_CHAN_INFO_INT_TIME
> > > for conversion time setting exclusively. Do we have something
> > > similar under hwmon?
> > >
> >
> > No. I think what you have should be good enough for now.
> > Please see comments below.
OK. I will go ahead with this approach then.
> > > + /* Update Bus-voltage conversion time */
> > > + ret = regmap_update_bits(ina->regmap, INA3221_CONFIG,
> > > + INA3221_CONFIG_VBUS_CT_MASK,
> > > + idx << INA3221_CONFIG_VBUS_CT_SHIFT);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + /* Update Shunt-voltage conversion time */
> > > + ret = regmap_update_bits(ina->regmap, INA3221_CONFIG,
> > > + INA3221_CONFIG_VSH_CT_MASK,
> > > + idx << INA3221_CONFIG_VSH_CT_SHIFT);
> > > + if (ret)
> > > + return ret;
> >
> > It should be possible to update both conversion times with a single call,
> > since both calls touch only one register. Something like
> >
> > ret = regmap_update_bits(ina->regmap, INA3221_CONFIG
> > INA3221_CONFIG_VBUS_CT_MASK | INA3221_CONFIG_VSH_CT_MASK,
> > (idx << INA3221_CONFIG_VBUS_CT_SHIFT) | (idx << INA3221_CONFIG_VSH_CT_SHIFT));
> >
> > Granted, that is a bit long, but it saves an extra i2c write operation.
Will merge them.
> Thinking about it ... does it even make sense to cache reg_config twice,
> or would it be better to just update the local copy and use regmap_write()
> to send it to the chip ?
I remember the reason of adding the read-back was to prevent race
condition. But now we have mutex protections for all sysfs nodes,
maybe it's not necessary anymore. I will read the code carefully
and see if it's safe to remove it -- will do in a separate patch.
Thanks
Nicolin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hwmon: (ina3221) Add voltage conversion time settings
2019-04-17 18:39 ` Nicolin Chen
@ 2019-04-17 19:48 ` Nicolin Chen
2019-04-17 20:12 ` Guenter Roeck
0 siblings, 1 reply; 7+ messages in thread
From: Nicolin Chen @ 2019-04-17 19:48 UTC (permalink / raw)
To: Guenter Roeck; +Cc: jdelvare, linux-hwmon, linux-kernel, corbet, linux-doc
On Wed, Apr 17, 2019 at 11:39:49AM -0700, Nicolin Chen wrote:
> > Thinking about it ... does it even make sense to cache reg_config twice,
> > or would it be better to just update the local copy and use regmap_write()
> > to send it to the chip ?
>
> I remember the reason of adding the read-back was to prevent race
> condition. But now we have mutex protections for all sysfs nodes,
> maybe it's not necessary anymore. I will read the code carefully
> and see if it's safe to remove it -- will do in a separate patch.
I just recalled a second thought for the reason why I left them
there as it'd logically require a copy to restore upon failure
of regmap_write, that might not look so neat as the read-back:
old_config = reg_config;
reg_config &= mask;
reg_config |= val;
ret = regmap_write(reg_config);
if (ret) {
reg_config = old_config;
return ret;
}
Would you prefer this?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hwmon: (ina3221) Add voltage conversion time settings
2019-04-17 19:48 ` Nicolin Chen
@ 2019-04-17 20:12 ` Guenter Roeck
2019-04-17 21:10 ` Nicolin Chen
0 siblings, 1 reply; 7+ messages in thread
From: Guenter Roeck @ 2019-04-17 20:12 UTC (permalink / raw)
To: Nicolin Chen; +Cc: jdelvare, linux-hwmon, linux-kernel, corbet, linux-doc
On Wed, Apr 17, 2019 at 12:48:18PM -0700, Nicolin Chen wrote:
> On Wed, Apr 17, 2019 at 11:39:49AM -0700, Nicolin Chen wrote:
>
> > > Thinking about it ... does it even make sense to cache reg_config twice,
> > > or would it be better to just update the local copy and use regmap_write()
> > > to send it to the chip ?
> >
> > I remember the reason of adding the read-back was to prevent race
> > condition. But now we have mutex protections for all sysfs nodes,
> > maybe it's not necessary anymore. I will read the code carefully
> > and see if it's safe to remove it -- will do in a separate patch.
>
> I just recalled a second thought for the reason why I left them
> there as it'd logically require a copy to restore upon failure
> of regmap_write, that might not look so neat as the read-back:
>
> old_config = reg_config;
> reg_config &= mask;
> reg_config |= val;
> ret = regmap_write(reg_config);
> if (ret) {
> reg_config = old_config;
> return ret;
> }
reg = (reg_config & mask) | val;
ret = regmap_write(reg);
if (ret)
return ret;
reg_config = reg;
doesn't look that bad to me, and is much less costly.
Guenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hwmon: (ina3221) Add voltage conversion time settings
2019-04-17 20:12 ` Guenter Roeck
@ 2019-04-17 21:10 ` Nicolin Chen
0 siblings, 0 replies; 7+ messages in thread
From: Nicolin Chen @ 2019-04-17 21:10 UTC (permalink / raw)
To: Guenter Roeck; +Cc: jdelvare, linux-hwmon, linux-kernel, corbet, linux-doc
On Wed, Apr 17, 2019 at 01:12:34PM -0700, Guenter Roeck wrote:
> On Wed, Apr 17, 2019 at 12:48:18PM -0700, Nicolin Chen wrote:
> > On Wed, Apr 17, 2019 at 11:39:49AM -0700, Nicolin Chen wrote:
> >
> > > > Thinking about it ... does it even make sense to cache reg_config twice,
> > > > or would it be better to just update the local copy and use regmap_write()
> > > > to send it to the chip ?
> > >
> > > I remember the reason of adding the read-back was to prevent race
> > > condition. But now we have mutex protections for all sysfs nodes,
> > > maybe it's not necessary anymore. I will read the code carefully
> > > and see if it's safe to remove it -- will do in a separate patch.
> >
> > I just recalled a second thought for the reason why I left them
> > there as it'd logically require a copy to restore upon failure
> > of regmap_write, that might not look so neat as the read-back:
> >
> > old_config = reg_config;
> > reg_config &= mask;
> > reg_config |= val;
> > ret = regmap_write(reg_config);
> > if (ret) {
> > reg_config = old_config;
> > return ret;
> > }
>
> reg = (reg_config & mask) | val;
> ret = regmap_write(reg);
> if (ret)
> return ret;
> reg_config = reg;
>
> doesn't look that bad to me, and is much less costly.
Okay. Will submit a change. Thanks
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-04-17 21:10 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-16 23:55 [PATCH] hwmon: (ina3221) Add voltage conversion time settings Nicolin Chen
2019-04-17 13:46 ` Guenter Roeck
2019-04-17 14:04 ` Guenter Roeck
2019-04-17 18:39 ` Nicolin Chen
2019-04-17 19:48 ` Nicolin Chen
2019-04-17 20:12 ` Guenter Roeck
2019-04-17 21:10 ` Nicolin Chen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).