[PATCH] hwmon: (ina3221) Add voltage conversion time settings

[PATCH] hwmon: (ina3221) Add voltage conversion time settings

[Nicolin Chen]

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

Re: [PATCH] hwmon: (ina3221) Add voltage conversion time settings

[Guenter Roeck]

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,
> 

Re: [PATCH] hwmon: (ina3221) Add voltage conversion time settings

[Guenter Roeck]

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,
>>
> 
> 

Re: [PATCH] hwmon: (ina3221) Add voltage conversion time settings

[Nicolin Chen]

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

Re: [PATCH] hwmon: (ina3221) Add voltage conversion time settings

[Nicolin Chen]

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?

Re: [PATCH] hwmon: (ina3221) Add voltage conversion time settings

[Guenter Roeck]

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

Re: [PATCH] hwmon: (ina3221) Add voltage conversion time settings

[Nicolin Chen]

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