[PATCH] hwmon: (ina3221) Fix read timeout issue
After introducing "samples" to the calculation of wait time, the
driver might timeout at the regmap_field_read_poll_timeout call,
because the wait time could be longer than the 10 usec limit
due to a large "samples" number.
So this patch sets the timeout limit to 2 times of the wait time
in order to fix this issue.
Fixes: 5c090abf945b ("hwmon: (ina3221) Add averaging mode support")
Signed-off-by: Nicolin Chen
---
drivers/hwmon/ina3221.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index ef37479991be..f335d0cb0c77 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -213,7 +213,7 @@ static inline int ina3221_wait_for_data(struct ina3221_data
*ina)
/* Polling the CVRF bit to make sure read data is ready */
return regmap_field_read_poll_timeout(ina->fields[F_CVRF],
- cvrf, cvrf, wait, 10);
+ cvrf, cvrf, wait, wait * 2);
}
static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
--
2.17.1
Re: ABI for these two registers?
On Wed, Oct 02, 2019 at 10:03:11PM -0700, Guenter Roeck wrote: > On 10/1/19 3:17 PM, Nicolin Chen wrote: > > Hello Guenter, > > > > It's been nearly three weeks. Would it be possible for you to > > provide some advice on my latest questions? I'd like to write > > code and submit changes once ABI is confirmed... > > > > Non-standard attribute discussions always require a lot of time, > since I have to re-read the chip documentation and try to understand > the reasoning why the attributes are needed. On top of that, > asking me "what should I do" instead of suggesting possible > solutions for me to choose from takes even more time, which > unfortunately I don't have right now. I'll try to get to it, > but, sorry, I can not promise you a time right now. Thank you for the reply...as I can't submit changes unless ABI is decided, I will be waiting for your further reply.
Re: ABI for these two registers?
Hello Guenter, It's been nearly three weeks. Would it be possible for you to provide some advice on my latest questions? I'd like to write code and submit changes once ABI is confirmed... Thank you! On Thu, Sep 12, 2019 at 05:12:38PM -0700, Nicolin Chen wrote: > > > > > > Datasheet: http://www.ti.com/lit/ds/symlink/ina3221.pdf > > > > > > (At page 32, chapter 8.6.2.14 and 8.6.2.15) > > > > > > > > > > > > I have two registers that I need to expose to user space: > > > > > > Shunt-Voltage-Sum and Shunt-Voltage-Limit registers > > > > > > > > > > > > Right now in[123]_input of INA3221 are for voltage channels > > > > > > while in[456]_input are for Shunt voltage channels. > > > > > > > > > > > > So can I just use in7_input and in7_crit for them? > > > > > > > > > > > Doesn't Shunt-Voltage-Limit apply to in[456]_input ? > > > > > If so, the limit should be attached to those. > > > > > > > > The initial patch of ina3221 driver applied Shunt-Voltage-Limits, > > > > being named as "Critical Alert Limit Registers" in the datasheet, > > > > to curr[123]_crit, corresponding to curr[123] and in[456]_input. > > > > > > > > And this Shunt-Voltage-Limit-Sum is more related to the reading > > > > from Shunt-Voltage-Sum, which we just agreed it to be in7_input. > > > > > > > You didn't say Shunt-Voltage-Limit-Sum earlier. You said > > > > Ahright...it's my fault. Sorry. > > > > > Shunt-Voltage-Limit. I would agree that Shunt-Voltage-Limit-Sum is > > > associated with Shunt-Voltage-Sum, but, again, that is not what you > > > said earlier. Confused :-( > > > > So, those two should be in7_input and in7_crit? > > And a couple of more questions upon it: > > 1) There are 3 control bits for the summation of this >shunt-voltage-sum register to enable corresponding >input channels. But in7_enable only allows users to >en/disable all three channels at the same time. So >how should I attach three enable bits using ABI? >Or should I create a device-specific sysfs node? > > 2) We have a requirement of providing current-sum and >current-limit-sum nodes, for use cases where those >enabled channels use same value shunt resistors. >It's similar to provide curr[1-3]_crit by dividing >shunt[123]_resistor from in[456]_input. So could I >deploy curr4_input and curr4_crit nodes for this >requirement?
Re: ABI for these two registers?
On Thu, Sep 12, 2019 at 03:45:29PM -0700, Nicolin Chen wrote: > On Thu, Sep 12, 2019 at 03:09:47PM -0700, Guenter Roeck wrote: > > On Thu, Sep 12, 2019 at 02:09:58PM -0700, Nicolin Chen wrote: > > > Hello Guenter, > > > > > > On Thu, Sep 12, 2019 at 11:32:18AM -0700, Guenter Roeck wrote: > > > > On Wed, Sep 11, 2019 at 05:28:14PM -0700, Nicolin Chen wrote: > > > > > Hello Guenter, > > > > > > > > > > Datasheet: http://www.ti.com/lit/ds/symlink/ina3221.pdf > > > > > (At page 32, chapter 8.6.2.14 and 8.6.2.15) > > > > > > > > > > I have two registers that I need to expose to user space: > > > > > Shunt-Voltage-Sum and Shunt-Voltage-Limit registers > > > > > > > > > > Right now in[123]_input of INA3221 are for voltage channels > > > > > while in[456]_input are for Shunt voltage channels. > > > > > > > > > > So can I just use in7_input and in7_crit for them? > > > > > > > > > Doesn't Shunt-Voltage-Limit apply to in[456]_input ? > > > > If so, the limit should be attached to those. > > > > > > The initial patch of ina3221 driver applied Shunt-Voltage-Limits, > > > being named as "Critical Alert Limit Registers" in the datasheet, > > > to curr[123]_crit, corresponding to curr[123] and in[456]_input. > > > > > > And this Shunt-Voltage-Limit-Sum is more related to the reading > > > from Shunt-Voltage-Sum, which we just agreed it to be in7_input. > > > > > You didn't say Shunt-Voltage-Limit-Sum earlier. You said > > Ahright...it's my fault. Sorry. > > > Shunt-Voltage-Limit. I would agree that Shunt-Voltage-Limit-Sum is > > associated with Shunt-Voltage-Sum, but, again, that is not what you > > said earlier. Confused :-( > > So, those two should be in7_input and in7_crit? And a couple of more questions upon it: 1) There are 3 control bits for the summation of this shunt-voltage-sum register to enable corresponding input channels. But in7_enable only allows users to en/disable all three channels at the same time. So how should I attach three enable bits using ABI? Or should I create a device-specific sysfs node? 2) We have a requirement of providing current-sum and current-limit-sum nodes, for use cases where those enabled channels use same value shunt resistors. It's similar to provide curr[1-3]_crit by dividing shunt[123]_resistor from in[456]_input. So could I deploy curr4_input and curr4_crit nodes for this requirement? Thank you!
Re: ABI for these two registers?
On Thu, Sep 12, 2019 at 03:09:47PM -0700, Guenter Roeck wrote: > On Thu, Sep 12, 2019 at 02:09:58PM -0700, Nicolin Chen wrote: > > Hello Guenter, > > > > On Thu, Sep 12, 2019 at 11:32:18AM -0700, Guenter Roeck wrote: > > > On Wed, Sep 11, 2019 at 05:28:14PM -0700, Nicolin Chen wrote: > > > > Hello Guenter, > > > > > > > > Datasheet: http://www.ti.com/lit/ds/symlink/ina3221.pdf > > > > (At page 32, chapter 8.6.2.14 and 8.6.2.15) > > > > > > > > I have two registers that I need to expose to user space: > > > > Shunt-Voltage-Sum and Shunt-Voltage-Limit registers > > > > > > > > Right now in[123]_input of INA3221 are for voltage channels > > > > while in[456]_input are for Shunt voltage channels. > > > > > > > > So can I just use in7_input and in7_crit for them? > > > > > > > Doesn't Shunt-Voltage-Limit apply to in[456]_input ? > > > If so, the limit should be attached to those. > > > > The initial patch of ina3221 driver applied Shunt-Voltage-Limits, > > being named as "Critical Alert Limit Registers" in the datasheet, > > to curr[123]_crit, corresponding to curr[123] and in[456]_input. > > > > And this Shunt-Voltage-Limit-Sum is more related to the reading > > from Shunt-Voltage-Sum, which we just agreed it to be in7_input. > > > You didn't say Shunt-Voltage-Limit-Sum earlier. You said Ahright...it's my fault. Sorry. > Shunt-Voltage-Limit. I would agree that Shunt-Voltage-Limit-Sum is > associated with Shunt-Voltage-Sum, but, again, that is not what you > said earlier. Confused :-( So, those two should be in7_input and in7_crit? Thanks!
Re: ABI for these two registers?
Hello Guenter, On Thu, Sep 12, 2019 at 11:32:18AM -0700, Guenter Roeck wrote: > On Wed, Sep 11, 2019 at 05:28:14PM -0700, Nicolin Chen wrote: > > Hello Guenter, > > > > Datasheet: http://www.ti.com/lit/ds/symlink/ina3221.pdf > > (At page 32, chapter 8.6.2.14 and 8.6.2.15) > > > > I have two registers that I need to expose to user space: > > Shunt-Voltage-Sum and Shunt-Voltage-Limit registers > > > > Right now in[123]_input of INA3221 are for voltage channels > > while in[456]_input are for Shunt voltage channels. > > > > So can I just use in7_input and in7_crit for them? > > > Doesn't Shunt-Voltage-Limit apply to in[456]_input ? > If so, the limit should be attached to those. The initial patch of ina3221 driver applied Shunt-Voltage-Limits, being named as "Critical Alert Limit Registers" in the datasheet, to curr[123]_crit, corresponding to curr[123] and in[456]_input. And this Shunt-Voltage-Limit-Sum is more related to the reading from Shunt-Voltage-Sum, which we just agreed it to be in7_input. Do you mean that this Limit-Sum should become in[456]_crit even though it only has one value, related to sum of all three shunt voltage channels? > in7_input for Shunt-Voltage-Sum would be fine. Okay. Will add in7_input for Shunt-Voltage-Sum. Thanks!
ABI for these two registers?
Hello Guenter, Datasheet: http://www.ti.com/lit/ds/symlink/ina3221.pdf (At page 32, chapter 8.6.2.14 and 8.6.2.15) I have two registers that I need to expose to user space: Shunt-Voltage-Sum and Shunt-Voltage-Limit registers Right now in[123]_input of INA3221 are for voltage channels while in[456]_input are for Shunt voltage channels. So can I just use in7_input and in7_crit for them? Thanks Nicolin
[PATCH v2 2/2] hwmon: (ina3221) Add voltage conversion time settings
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
---
Changelog
v1->v2:
* Merged two adjacent calls of regmap_update_bits().
* Replaced CONFIG register read-back with tmp variable
Documentation/hwmon/ina3221 | 9 ++
drivers/hwmon/ina3221.c | 58 -
2 files changed, 59 insertions(+), 8 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 62040aac653c..e0637fed9585 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(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(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 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;
}
@@ -322,6 +345,23 @@ static int ina3221_write_chip(struct device *dev, u32
attr, long val)
if (ret)
return ret;
+ /* Update reg_config accordingly */
+ ina->reg_config = t
[PATCH v2 0/2] hwmon: (ina3221) Two changes for INA3221
Adding two changes for INA3221 HWMON driver. Changlog v1->v2: * Added PATCH-1 * Rebased PATCH-2 (see detail of changelog in it) Nicolin Chen (2): hwmon: (ina3221) Do not read-back to cache reg_config hwmon: (ina3221) Add voltage conversion time settings Documentation/hwmon/ina3221 | 9 + drivers/hwmon/ina3221.c | 77 + 2 files changed, 69 insertions(+), 17 deletions(-) -- 2.17.1
Re: [PATCH] hwmon: (ina3221) Add voltage conversion time settings
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
Re: [PATCH] hwmon: (ina3221) Add voltage conversion time settings
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
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
[PATCH] hwmon: (ina3221) Add voltage conversion time settings
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
---
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..af4ab8ce 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_re
[PATCH v2] hwmon: (ina3221) Add averaging mode support
The CONFIG register has a 3-bit averaging mode field for users
to setup the number of samples that are collected and averaged
together. This is very useful to filter noise from sensor data.
This patch adds a 'samples' sysfs node using hwmon_chip_samples
of hwmon core, and updates wait time calculation by taking this
samples value into account.
Signed-off-by: Nicolin Chen
---
Changelog
v1->v2:
* Implemented "samples" through hwmon_chip_info
* Used find_closest()
Documentation/hwmon/ina3221 | 3 ++
drivers/hwmon/ina3221.c | 67 -
2 files changed, 69 insertions(+), 1 deletion(-)
diff --git a/Documentation/hwmon/ina3221 b/Documentation/hwmon/ina3221
index 4b82cbfb551c..ed3f22769d4b 100644
--- a/Documentation/hwmon/ina3221
+++ b/Documentation/hwmon/ina3221
@@ -35,3 +35,6 @@ curr[123]_max Warning alert current(mA) setting,
activates the
average is above this value.
curr[123]_max_alarm Warning alert current limit exceeded
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
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index e6f43df0435c..74d39d212931 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -22,6 +22,7 @@
#include
#include
#include
+#include
#define INA3221_DRIVER_NAME"ina3221"
@@ -51,6 +52,9 @@
#define INA3221_CONFIG_VBUS_CT_SHIFT 6
#define INA3221_CONFIG_VBUS_CT_MASKGENMASK(8, 6)
#define INA3221_CONFIG_VBUS_CT(x) (((x) & GENMASK(8, 6)) >> 6)
+#define INA3221_CONFIG_AVG_SHIFT 9
+#define INA3221_CONFIG_AVG_MASKGENMASK(11, 9)
+#define INA3221_CONFIG_AVG(x) (((x) & GENMASK(11, 9)) >> 9)
#define INA3221_CONFIG_CHs_EN_MASK GENMASK(14, 12)
#define INA3221_CONFIG_CHx_EN(x) BIT(14 - (x))
@@ -135,17 +139,24 @@ static const u16 ina3221_conv_time[] = {
140, 204, 332, 588, 1100, 2116, 4156, 8244,
};
+/* Lookup table for number of samples using in averaging mode */
+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)
{
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 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);
+ wait = channels * (vbus_ct + vsh_ct) * samples;
/* Polling the CVRF bit to make sure read data is ready */
return regmap_field_read_poll_timeout(ina->fields[F_CVRF],
@@ -176,6 +187,21 @@ static const u8 ina3221_in_reg[] = {
INA3221_SHUNT3,
};
+static int ina3221_read_chip(struct device *dev, u32 attr, long *val)
+{
+ struct ina3221_data *ina = dev_get_drvdata(dev);
+ int regval;
+
+ switch (attr) {
+ case hwmon_chip_samples:
+ regval = INA3221_CONFIG_AVG(ina->reg_config);
+ *val = ina3221_avg_samples[regval];
+ return 0;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
static int ina3221_read_in(struct device *dev, u32 attr, int channel, long
*val)
{
const bool is_shunt = channel > INA3221_CHANNEL3;
@@ -279,6 +305,30 @@ 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;
+
+ switch (attr) {
+ case hwmon_chip_samples:
+ idx = find_closest(val, ina3221_avg_samples,
+ ARRAY_SIZE(ina3221_avg_samples));
+
+ ret = regmap_update_bits(ina->regmap, INA3221_CONFIG,
+INA3221_CONFIG_AVG_MASK,
+idx << INA3221_CONFIG_AVG_SHIFT);
+ if (ret)
+ return ret;
+
+ /* Update reg_config accordingly */
+ return regmap_read(ina->regmap, INA3221_CONFIG,
+ >reg_config);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
static int ina3221_write_curr(struct device *dev, u32 attr,
int channel, long val)
{
@@ -361,6 +411,9 @@ static int ina32
Re: [PATCH] hwmon: Add support for samples attributes
Thanks for adding this.
On Mon, Apr 15, 2019 at 01:27:12PM -0700, Guenter Roeck wrote:
> Add support for the new samples attributes to the hwmon core.
>
> Cc: Krzysztof Adamski
> Cc: Nicolin Chen
Acked-by: Nicolin Chen
Will redo my change and test through API later.
> Signed-off-by: Guenter Roeck
> ---
> drivers/hwmon/hwmon.c | 5 +
> include/linux/hwmon.h | 11 +++
> 2 files changed, 16 insertions(+)
> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
> index 7a8cc06a0d61..7b18a6150a11 100644
> --- a/include/linux/hwmon.h
> +++ b/include/linux/hwmon.h
> @@ -40,6 +40,12 @@ enum hwmon_chip_attributes {
> hwmon_chip_register_tz,
> hwmon_chip_update_interval,
> hwmon_chip_alarms,
> + hwmon_chip_samples,
> + hwmon_chip_curr_samples,
> + hwmon_chip_in_samples,
> + hwmon_chip_power_samples,
> + hwmon_chip_temp_samples,
> +
> };
Why add a blank line at the end?
Re: [PATCH 2/3] lm25066: export sysfs attribute for SAMPLES_FOR_AVG
On Wed, Apr 10, 2019 at 05:55:19PM -0700, Guenter Roeck wrote:
> > +static ssize_t samples_for_avg_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev->parent);
> > + int ret;
> > +
> > + ret = pmbus_read_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return sprintf(buf, "%d\n", 1 << ret);
> > +}
> > +
> > +static ssize_t samples_for_avg_store(struct device *dev,
> > +struct device_attribute *attr,
> > +const char *buf, size_t count)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev->parent);
> > + int ret, val;
> > +
> > + ret = kstrtoint(buf, 0, );
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = pmbus_write_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG,
> > + ilog2(val));
> > + if (ret < 0)
> > + return ret;
> > +
> > + return count;
> > +}
> > +
> > +static DEVICE_ATTR_RW(samples_for_avg);
> > +
> > +static struct attribute *lm25056_attrs[] = {
> > + _attr_samples_for_avg.attr,
> > + NULL,
> > +};
> > +ATTRIBUTE_GROUPS(lm25056); // should we set a name of this group to put all
> > + // those attributes in subdirectory? Like "custom" ?
> > +
> We don't add subdirectories for other chips, and we won't start it here.
>
> I don't mind the attribute itself, but I do mind its name. We'll have
> to find something more generic, such as 'num_samples' or just 'samples'.
> I am open to suggestions. We'll also have to decide if the attribute(s)
> should be limited to one per chip, or if there can be multiple attributes.
> For example, MAX34462 has separate values for iout_samples and adc_average.
> Do we want samples, {curr,in,power,...}_samples, or both ? Or even
> currX_samples ?
For my use case -- TI's INA chips, there is only one "samples"
configuration being used for all currX_inputs and inX_inputs.
So having a "samples" node would certainly fit better than an
in0_samples. So I vote for having both.
Thank you
Nicolin
Re: [PATCH] hwmon: (ina3221) Add averaging mode support
On Tue, Mar 12, 2019 at 03:37:59PM -0700, Guenter Roeck wrote: > > +average Averaging mode. Supports the list of number of > > samples: > > + 1, 4, 16, 64, 128, 256, 512, 1024 > > This is the number of samples, so I think "samples" would be a better > attribute name. This would also avoid confusion with other _average > attributes. > > I'll need to check with other chips if this is the best approach and > name for the attribute, especially to see if it should be chip-wide > or per sensor. Will wait for that then. Another thing is that the conversion times are also configurable. And I plan to use the update_interval in the ABI once this average is added. This means that average value will be changed via this 'average' node (or other name), and conversion times will be changed via the 'update_interval' node by following the formula: update_interval = channels * (vbus_ct + vsh_ct) * avg > > + for (i = 0; i < ARRAY_SIZE(ina3221_avg); i++) > > + if (ina3221_avg[i] == avg) > > + break; > > Please use find_closest(). OK. Thanks
[PATCH] hwmon: (ina3221) Add averaging mode support
The CONFIG register has a 3-bit averaging mode field for users
to setup the number of samples that are collected and averaged
together. This is very useful to filter noise from sensor data.
This patch adds an 'average' sysfs node, and updates wait time
calculation by taking this average value into account.
Signed-off-by: Nicolin Chen
---
Documentation/hwmon/ina3221 | 2 ++
drivers/hwmon/ina3221.c | 61 -
2 files changed, 62 insertions(+), 1 deletion(-)
diff --git a/Documentation/hwmon/ina3221 b/Documentation/hwmon/ina3221
index 4b82cbfb551c..1e25be009d24 100644
--- a/Documentation/hwmon/ina3221
+++ b/Documentation/hwmon/ina3221
@@ -35,3 +35,5 @@ curr[123]_max Warning alert current(mA) setting,
activates the
average is above this value.
curr[123]_max_alarm Warning alert current limit exceeded
in[456]_input Shunt voltage(uV) for channels 1, 2, and 3 respectively
+average Averaging mode. Supports the list of number of samples:
+ 1, 4, 16, 64, 128, 256, 512, 1024
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 3626b87a5fd2..259c192427ac 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -51,6 +51,9 @@
#define INA3221_CONFIG_VBUS_CT_SHIFT 6
#define INA3221_CONFIG_VBUS_CT_MASKGENMASK(8, 6)
#define INA3221_CONFIG_VBUS_CT(x) (((x) & GENMASK(8, 6)) >> 6)
+#define INA3221_CONFIG_AVG_SHIFT 9
+#define INA3221_CONFIG_AVG_MASKGENMASK(11, 9)
+#define INA3221_CONFIG_AVG(x) (((x) & GENMASK(11, 9)) >> 9)
#define INA3221_CONFIG_CHs_EN_MASK GENMASK(14, 12)
#define INA3221_CONFIG_CHx_EN(x) BIT(14 - (x))
@@ -135,17 +138,24 @@ static const u16 ina3221_conv_time[] = {
140, 204, 332, 588, 1100, 2116, 4156, 8244,
};
+/* Lookup table for averaging rates */
+static const int ina3221_avg[] = {
+ 1, 4, 16, 64, 128, 256, 512, 1024,
+};
+
static inline int ina3221_wait_for_data(struct ina3221_data *ina)
{
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 avg_idx = INA3221_CONFIG_AVG(ina->reg_config);
u32 vbus_ct = ina3221_conv_time[vbus_ct_idx];
u32 vsh_ct = ina3221_conv_time[vsh_ct_idx];
+ u32 avg = ina3221_avg[avg_idx];
u32 wait, cvrf;
/* Calculate total conversion time */
- wait = channels * (vbus_ct + vsh_ct);
+ wait = channels * (vbus_ct + vsh_ct) * avg;
/* Polling the CVRF bit to make sure read data is ready */
return regmap_field_read_poll_timeout(ina->fields[F_CVRF],
@@ -545,15 +555,64 @@ static ssize_t ina3221_shunt_store(struct device *dev,
return count;
}
+static ssize_t ina3221_avg_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct ina3221_data *ina = dev_get_drvdata(dev);
+ u32 avg_idx = INA3221_CONFIG_AVG(ina->reg_config);
+
+ return snprintf(buf, PAGE_SIZE, "%d\n", ina3221_avg[avg_idx]);
+}
+
+static ssize_t ina3221_avg_store(struct device *dev,
+struct device_attribute *attr,
+const char *buf, size_t count)
+{
+ struct ina3221_data *ina = dev_get_drvdata(dev);
+ int ret, avg, i;
+
+ ret = kstrtoint(buf, 0, );
+ if (ret)
+ return ret;
+
+ for (i = 0; i < ARRAY_SIZE(ina3221_avg); i++)
+ if (ina3221_avg[i] == avg)
+ break;
+
+ if (i == ARRAY_SIZE(ina3221_avg))
+ return -EINVAL;
+
+ mutex_lock(>lock);
+
+ ret = regmap_update_bits(ina->regmap, INA3221_CONFIG,
+INA3221_CONFIG_AVG_MASK,
+i << INA3221_CONFIG_AVG_SHIFT);
+ if (ret)
+ goto unlock;
+
+ /* Update reg_config accordingly */
+ ret = regmap_read(ina->regmap, INA3221_CONFIG, >reg_config);
+ if (ret)
+ goto unlock;
+
+unlock:
+ mutex_unlock(>lock);
+
+ return ret ? ret : count;
+}
+
/* shunt resistance */
static SENSOR_DEVICE_ATTR_RW(shunt1_resistor, ina3221_shunt, INA3221_CHANNEL1);
static SENSOR_DEVICE_ATTR_RW(shunt2_resistor, ina3221_shunt, INA3221_CHANNEL2);
static SENSOR_DEVICE_ATTR_RW(shunt3_resistor, ina3221_shunt, INA3221_CHANNEL3);
+/* averaging mode */
+static SENSOR_DEVICE_ATTR_RW(average, ina3221_avg, 0);
static struct attribute *ina3221_attrs[] = {
_dev_attr_shunt1_resistor.dev_attr.attr,
_dev_attr_shunt2_resistor.dev_attr.attr,
_dev_attr_shunt3_resistor.dev_attr.attr,
+ _dev_attr_average.dev_attr.attr,
NULL,
};
ATTRIBUTE_GROUPS(ina3221);
--
2.17.1
Re: [RFC][PATCH] hwmon: (ina2xx) Improve current and power reading precision
Hi Guenter, On Thu, Jan 17, 2019 at 03:13:23PM -0800, Guenter Roeck wrote: > I have one claim stating that your change won't make a difference, > and your claim that it would. That leaves me with no choice but to > spend a large amount of time with the datasheet, and possibly with > my evaluation boards, trying to find out who is correct. Unfortunately, > time is scarce to come by those days. I would very much prefer for > you folks to sort out your differences and present me with a single > opinion. > > Long term the best solution would really be to replace the hwmon > driver with the iio driver and use the iio-hwmon bridge. The ina2xx > driver doesn't support limits, so there is no real benefit of using > the hwmon driver over the iio driver, at least assuming that the > iio driver supports all the attributes. So maybe you should make your > case for the changes in the iio driver. Thanks for the input. Let's drop this hwmon change then.
[PATCH v3 1/2] dt-bindings: hwmon: ina3221: Add ti,single-shot property
By default, ina3221, as a hardware monitor, continuously measures the inputs and generates corresponding data. However, for battery powered devices, this mode might be power consuming. This patch adds a "ti,single-shot" property to allow changing the default continuous mode to single-shot operating mode. Signed-off-by: Nicolin Chen Reviewed-by: Rob Herring --- Changelog v2->v3: * Added "Reviewed-by" from Rob v1->v2: * N/A Documentation/devicetree/bindings/hwmon/ina3221.txt | 10 ++ 1 file changed, 10 insertions(+) diff --git a/Documentation/devicetree/bindings/hwmon/ina3221.txt b/Documentation/devicetree/bindings/hwmon/ina3221.txt index a7b25caa2b8e..fa63b6171407 100644 --- a/Documentation/devicetree/bindings/hwmon/ina3221.txt +++ b/Documentation/devicetree/bindings/hwmon/ina3221.txt @@ -6,6 +6,16 @@ Texas Instruments INA3221 Device Tree Bindings - reg: I2C address Optional properties: + - ti,single-shot: This chip has two power modes: single-shot (chip takes one +measurement and then shuts itself down) and continuous ( +chip takes continuous measurements). The continuous mode is +more reliable and suitable for hardware monitor type device, +but the single-shot mode is more power-friendly and useful +for battery-powered device which cares power consumptions +while still needs some measurements occasionally. +If this property is present, the single-shot mode will be +used, instead of the default continuous one for monitoring. + = The node contains optional child nodes for three channels = = Each child node describes the information of input source = -- 2.17.1
[PATCH v3 2/2] hwmon: (ina3221) Implement ti,single-shot DT property
By default, ina3221, as a hardware monitor, continuously measures
the inputs and generates corresponding data. However, for battery
powered devices, this mode might be power consuming.
The DT binding doc is updated with a new boolean type property to
allow changing the default operating mode from consuming mode to
single-shot mode, which will measure input on demand and then shut
down to save power.
So this patch implements the DT property accordingly.
Signed-off-by: Nicolin Chen
---
Changelog
v2->v3:
* Dropped useless if condition by using the return value directly.
v1->v2:
* Replaced the useless mode defines with a boolean variable.
drivers/hwmon/ina3221.c | 19 +++
1 file changed, 19 insertions(+)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index e90ccac8bebb..a1e94d5d1e41 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -111,6 +111,7 @@ struct ina3221_input {
* @inputs: Array of channel input source specific structures
* @lock: mutex lock to serialize sysfs attribute accesses
* @reg_config: Register value of INA3221_CONFIG
+ * @single_shot: running in single-shot operating mode
*/
struct ina3221_data {
struct device *pm_dev;
@@ -119,6 +120,8 @@ struct ina3221_data {
struct ina3221_input inputs[INA3221_NUM_CHANNELS];
struct mutex lock;
u32 reg_config;
+
+ bool single_shot;
};
static inline bool ina3221_is_enabled(struct ina3221_data *ina, int channel)
@@ -188,6 +191,11 @@ static int ina3221_read_in(struct device *dev, u32 attr,
int channel, long *val)
if (!ina3221_is_enabled(ina, channel))
return -ENODATA;
+ /* Write CONFIG register to trigger a single-shot measurement */
+ if (ina->single_shot)
+ regmap_write(ina->regmap, INA3221_CONFIG,
+ina->reg_config);
+
ret = ina3221_wait_for_data(ina);
if (ret)
return ret;
@@ -232,6 +240,11 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
if (!ina3221_is_enabled(ina, channel))
return -ENODATA;
+ /* Write CONFIG register to trigger a single-shot measurement */
+ if (ina->single_shot)
+ regmap_write(ina->regmap, INA3221_CONFIG,
+ina->reg_config);
+
ret = ina3221_wait_for_data(ina);
if (ret)
return ret;
@@ -617,6 +630,8 @@ static int ina3221_probe_from_dt(struct device *dev, struct
ina3221_data *ina)
if (!np)
return 0;
+ ina->single_shot = of_property_read_bool(np, "ti,single-shot");
+
for_each_child_of_node(np, child) {
ret = ina3221_probe_child_from_dt(dev, child, ina);
if (ret)
@@ -666,6 +681,10 @@ static int ina3221_probe(struct i2c_client *client,
/* The driver will be reset, so use reset value */
ina->reg_config = INA3221_CONFIG_DEFAULT;
+ /* Clear continuous bit to use single-shot mode */
+ if (ina->single_shot)
+ ina->reg_config &= ~INA3221_CONFIG_MODE_CONTINUOUS;
+
/* Disable channels if their inputs are disconnected */
for (i = 0; i < INA3221_NUM_CHANNELS; i++) {
if (ina->inputs[i].disconnected)
--
2.17.1
[PATCH v3 0/2] hwmon (ina3221) Add single-shot mode support
By default, ina3221, as a hardware monitor, continuously measures the inputs and generates corresponding data. However, for battery powered devices, this mode might be power consuming. This series of patches add a feature of changing default operating mode to its single-shot mode via DT. Changelog v2->v3: * Added "Reviewed-by" from Rob to PATCH-1 * Cleaned-up PATCH-2 v1->v2: * Cleaned-up PATCH-2 Nicolin Chen (2): dt-bindings: hwmon: ina3221: Add ti,single-shot property hwmon: (ina3221) Implement ti,single-shot DT property .../devicetree/bindings/hwmon/ina3221.txt | 10 ++ drivers/hwmon/ina3221.c | 19 +++ 2 files changed, 29 insertions(+) -- 2.17.1
Re: [PATCH v2 2/2] hwmon: (ina3221) Implement ti,single-shot DT property
On Thu, Jan 17, 2019 at 02:58:42PM -0800, Guenter Roeck wrote: > "Right now" was supposed to mean that you should wait for Rob's > feedback before sending v3 with the feedback above applied. > Did you send that version ? I don't see it in patchwork, nor > in my inbox. Oh, sorry. Will resend it. Thanks for the reply.
Re: [RFC][PATCH] hwmon: (ina2xx) Improve current and power reading precision
On Fri, Jan 04, 2019 at 05:26:42PM -0800, Nicolin Chen wrote: > Hi Stefan, > > Sorry for a super late reply. I took a long vacation. > > On Wed, Nov 21, 2018 at 10:16:09PM +, Brüns, Stefan wrote: > > > > Another concern may be voltage drop over the shunt, but for this case > > > > you > > > > have a nominal voltage of 1.8V, so 30uV are 0.001%. > > > > > > > > > When measuring a 1.8v voltage running a small current (e.g. 33 mA), > > > > > the power value (that's supposed to be 59.4 mW) becomes inaccurate > > > > > due to the larger scale (25mA for method A; 62.5 mA for method B). > > > > > > Just found out that I have typos here: 25mW and 62.5mW. > > > > > > > Another look into the datasheet reveals, even at full gain (PGA=1), the > > > > LSB is 40mV / 2^12 = 40mV / 4096 ~ 10uV. So when the current ADC reads > > > > out as 3*LSB, this anything between 25mA and 35mA. This is the best case > > > > figure. > > > Current read doesn't get affected a lot actually, since hwmon ABI > > > also reports current value in unit mA. However, the power read is > > > the matter here. With a 62.5mW power_lsb, power results are kinda > > > useless on my system. > > > > The reported current does not matter here, actually. Internally, the ADC > > value > > will have an uncertainty of 10mA (at PGA=1). At 1.8V, your uncertainty is > > 18mW. And thats *only* the quantization noise. It wont get better than that. > > The fact is that I do get better power results after setting the > calibration value to 0x7ff. That's the necessity of this change. > > > Also note, you are apparently using the ina2xx hwmon driver - I strongly > > advise against it, you should either use the ina2xx driver from the IIO > > subsystem directly, or use the IIO driver via iio-hwmon. > > The IIO version is also using the minimum calibration value. It > will not solve my problem here. > > > There is also always the possibility to read the bus and shunt voltage > > registers and calculate the power manually. > > Won't that be a waste since the hardware could have provided a > better accuracy? It would need more I2C bus reads and cpu cycles > for calculation. > > I don't get why you're against a setting for calibration value. > This is how the hardware got designed to cover different cases. > Since we do have such a case that needs some accuracy, it'd be > fair to add it into the driver. Plus, the feature won't change > the minimum calibration value at all -- everyone would be happy. Stefan, Would you please kindly give an ack to this intention so that we can at least move forward for patch review? Neither changing hardware resistor values nor simply ignoring the inaccuracy is acceptable for us. Since configuring that calibration register value can help our use cases, we really need this setting to be available in the driver. Guenter, Do you have any input regarding this change? I would like to hear an opinion from you. Thank you
Re: [PATCH v2 2/2] hwmon: (ina3221) Implement ti,single-shot DT property
On Mon, Jan 07, 2019 at 11:35:55AM -0800, Guenter Roeck wrote: > > + if (of_property_read_bool(np, "ti,single-shot")) > > + ina->single_shot = true; > > + > ina->single_shot = of_property_read_bool(np, "ti,single-shot"); > > No need to resend right now; let's wait for feedback from Rob. Looks like Rob has acked. Is this resend version fine for review? Thank you Nicolin
[PATCH v2 0/2] hwmon (ina3221) Add single-shot mode support
By default, ina3221, as a hardware monitor, continuously measures the inputs and generates corresponding data. However, for battery powered devices, this mode might be power consuming. This series of patches add a feature of changing default operating mode to its single-shot mode via DT. Changelog v1->v2: * Cleaned-up PATCH-2 Nicolin Chen (2): dt-bindings: hwmon: ina3221: Add ti,single-shot property hwmon: (ina3221) Implement ti,single-shot DT property .../devicetree/bindings/hwmon/ina3221.txt | 10 ++ drivers/hwmon/ina3221.c | 20 +++ 2 files changed, 30 insertions(+) -- 2.17.1
[PATCH v2 1/2] dt-bindings: hwmon: ina3221: Add ti,single-shot property
By default, ina3221, as a hardware monitor, continuously measures the inputs and generates corresponding data. However, for battery powered devices, this mode might be power consuming. This patch adds a "ti,single-shot" property to allow changing the default continuous mode to single-shot operating mode. Signed-off-by: Nicolin Chen --- Documentation/devicetree/bindings/hwmon/ina3221.txt | 10 ++ 1 file changed, 10 insertions(+) diff --git a/Documentation/devicetree/bindings/hwmon/ina3221.txt b/Documentation/devicetree/bindings/hwmon/ina3221.txt index a7b25caa2b8e..fa63b6171407 100644 --- a/Documentation/devicetree/bindings/hwmon/ina3221.txt +++ b/Documentation/devicetree/bindings/hwmon/ina3221.txt @@ -6,6 +6,16 @@ Texas Instruments INA3221 Device Tree Bindings - reg: I2C address Optional properties: + - ti,single-shot: This chip has two power modes: single-shot (chip takes one +measurement and then shuts itself down) and continuous ( +chip takes continuous measurements). The continuous mode is +more reliable and suitable for hardware monitor type device, +but the single-shot mode is more power-friendly and useful +for battery-powered device which cares power consumptions +while still needs some measurements occasionally. +If this property is present, the single-shot mode will be +used, instead of the default continuous one for monitoring. + = The node contains optional child nodes for three channels = = Each child node describes the information of input source = -- 2.17.1
[PATCH v2 2/2] hwmon: (ina3221) Implement ti,single-shot DT property
By default, ina3221, as a hardware monitor, continuously measures
the inputs and generates corresponding data. However, for battery
powered devices, this mode might be power consuming.
The DT binding doc is updated with a new boolean type property to
allow changing the default operating mode from consuming mode to
single-shot mode, which will measure input on demand and then shut
down to save power.
So this patch implements the DT property accordingly.
Signed-off-by: Nicolin Chen
---
Changelog
v1->v2:
* Replaced the useless mode defines with a boolean variable.
drivers/hwmon/ina3221.c | 20
1 file changed, 20 insertions(+)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index e90ccac8bebb..4258a6ebe195 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -111,6 +111,7 @@ struct ina3221_input {
* @inputs: Array of channel input source specific structures
* @lock: mutex lock to serialize sysfs attribute accesses
* @reg_config: Register value of INA3221_CONFIG
+ * @single_shot: running in single-shot operating mode
*/
struct ina3221_data {
struct device *pm_dev;
@@ -119,6 +120,8 @@ struct ina3221_data {
struct ina3221_input inputs[INA3221_NUM_CHANNELS];
struct mutex lock;
u32 reg_config;
+
+ bool single_shot;
};
static inline bool ina3221_is_enabled(struct ina3221_data *ina, int channel)
@@ -188,6 +191,11 @@ static int ina3221_read_in(struct device *dev, u32 attr,
int channel, long *val)
if (!ina3221_is_enabled(ina, channel))
return -ENODATA;
+ /* Write CONFIG register to trigger a single-shot measurement */
+ if (ina->single_shot)
+ regmap_write(ina->regmap, INA3221_CONFIG,
+ina->reg_config);
+
ret = ina3221_wait_for_data(ina);
if (ret)
return ret;
@@ -232,6 +240,11 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
if (!ina3221_is_enabled(ina, channel))
return -ENODATA;
+ /* Write CONFIG register to trigger a single-shot measurement */
+ if (ina->single_shot)
+ regmap_write(ina->regmap, INA3221_CONFIG,
+ina->reg_config);
+
ret = ina3221_wait_for_data(ina);
if (ret)
return ret;
@@ -617,6 +630,9 @@ static int ina3221_probe_from_dt(struct device *dev, struct
ina3221_data *ina)
if (!np)
return 0;
+ if (of_property_read_bool(np, "ti,single-shot"))
+ ina->single_shot = true;
+
for_each_child_of_node(np, child) {
ret = ina3221_probe_child_from_dt(dev, child, ina);
if (ret)
@@ -666,6 +682,10 @@ static int ina3221_probe(struct i2c_client *client,
/* The driver will be reset, so use reset value */
ina->reg_config = INA3221_CONFIG_DEFAULT;
+ /* Clear continuous bit to use single-shot mode */
+ if (ina->single_shot)
+ ina->reg_config &= ~INA3221_CONFIG_MODE_CONTINUOUS;
+
/* Disable channels if their inputs are disconnected */
for (i = 0; i < INA3221_NUM_CHANNELS; i++) {
if (ina->inputs[i].disconnected)
--
2.17.1
Re: [PATCH 2/2] hwmon: (ina3221) Implement ti,single-shot DT property
On Fri, Jan 04, 2019 at 06:37:55PM -0800, Guenter Roeck wrote:
> > +enum ina3221_modes {
> > + INA3221_MODE_SINGLE_SHOT,
> > + INA3221_MODE_CONTINUOUS,
> > + INA3221_NUM_MODES,
>
> Is NUM_MODES used anywhere ? Please drop unless there is a reason to keep it.
Will clean it up in v2.
Thanks
Nicolin
Re: [RFC][PATCH] hwmon: (ina2xx) Improve current and power reading precision
Hi Stefan, Sorry for a super late reply. I took a long vacation. On Wed, Nov 21, 2018 at 10:16:09PM +, Brüns, Stefan wrote: > > > Another concern may be voltage drop over the shunt, but for this case you > > > have a nominal voltage of 1.8V, so 30uV are 0.001%. > > > > > > > When measuring a 1.8v voltage running a small current (e.g. 33 mA), > > > > the power value (that's supposed to be 59.4 mW) becomes inaccurate > > > > due to the larger scale (25mA for method A; 62.5 mA for method B). > > > > Just found out that I have typos here: 25mW and 62.5mW. > > > > > Another look into the datasheet reveals, even at full gain (PGA=1), the > > > LSB is 40mV / 2^12 = 40mV / 4096 ~ 10uV. So when the current ADC reads > > > out as 3*LSB, this anything between 25mA and 35mA. This is the best case > > > figure. > > Current read doesn't get affected a lot actually, since hwmon ABI > > also reports current value in unit mA. However, the power read is > > the matter here. With a 62.5mW power_lsb, power results are kinda > > useless on my system. > > The reported current does not matter here, actually. Internally, the ADC > value > will have an uncertainty of 10mA (at PGA=1). At 1.8V, your uncertainty is > 18mW. And thats *only* the quantization noise. It wont get better than that. The fact is that I do get better power results after setting the calibration value to 0x7ff. That's the necessity of this change. > Also note, you are apparently using the ina2xx hwmon driver - I strongly > advise against it, you should either use the ina2xx driver from the IIO > subsystem directly, or use the IIO driver via iio-hwmon. The IIO version is also using the minimum calibration value. It will not solve my problem here. > There is also always the possibility to read the bus and shunt voltage > registers and calculate the power manually. Won't that be a waste since the hardware could have provided a better accuracy? It would need more I2C bus reads and cpu cycles for calculation. I don't get why you're against a setting for calibration value. This is how the hardware got designed to cover different cases. Since we do have such a case that needs some accuracy, it'd be fair to add it into the driver. Plus, the feature won't change the minimum calibration value at all -- everyone would be happy. Thanks Nicolin
[PATCH 1/2] dt-bindings: hwmon: ina3221: Add ti,single-shot property
By default, ina3221, as a hardware monitor, continuously measures the inputs and generates corresponding data. However, for battery powered devices, this mode might be power consuming. This patch adds a "ti,single-shot" property to allow changing the default continuous mode to single-shot operating mode. Signed-off-by: Nicolin Chen --- Documentation/devicetree/bindings/hwmon/ina3221.txt | 10 ++ 1 file changed, 10 insertions(+) diff --git a/Documentation/devicetree/bindings/hwmon/ina3221.txt b/Documentation/devicetree/bindings/hwmon/ina3221.txt index a7b25caa2b8e..fa63b6171407 100644 --- a/Documentation/devicetree/bindings/hwmon/ina3221.txt +++ b/Documentation/devicetree/bindings/hwmon/ina3221.txt @@ -6,6 +6,16 @@ Texas Instruments INA3221 Device Tree Bindings - reg: I2C address Optional properties: + - ti,single-shot: This chip has two power modes: single-shot (chip takes one +measurement and then shuts itself down) and continuous ( +chip takes continuous measurements). The continuous mode is +more reliable and suitable for hardware monitor type device, +but the single-shot mode is more power-friendly and useful +for battery-powered device which cares power consumptions +while still needs some measurements occasionally. +If this property is present, the single-shot mode will be +used, instead of the default continuous one for monitoring. + = The node contains optional child nodes for three channels = = Each child node describes the information of input source = -- 2.17.1
[PATCH 2/2] hwmon: (ina3221) Implement ti,single-shot DT property
By default, ina3221, as a hardware monitor, continuously measures
the inputs and generates corresponding data. However, for battery
powered devices, this mode might be power consuming.
The DT binding doc is updated with a new boolean type property to
allow changing the default operating mode from consuming mode to
single-shot mode, which will measure input on demand and then shut
down to save power.
So this patch implements the DT property accordingly.
Signed-off-by: Nicolin Chen
---
drivers/hwmon/ina3221.c | 28
1 file changed, 28 insertions(+)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index e90ccac8bebb..152735659e19 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -91,6 +91,12 @@ enum ina3221_channels {
INA3221_NUM_CHANNELS
};
+enum ina3221_modes {
+ INA3221_MODE_SINGLE_SHOT,
+ INA3221_MODE_CONTINUOUS,
+ INA3221_NUM_MODES,
+};
+
/**
* struct ina3221_input - channel input source specific information
* @label: label of channel input source
@@ -111,6 +117,7 @@ struct ina3221_input {
* @inputs: Array of channel input source specific structures
* @lock: mutex lock to serialize sysfs attribute accesses
* @reg_config: Register value of INA3221_CONFIG
+ * @mode: Operating mode -- continuous or single-shot
*/
struct ina3221_data {
struct device *pm_dev;
@@ -119,6 +126,7 @@ struct ina3221_data {
struct ina3221_input inputs[INA3221_NUM_CHANNELS];
struct mutex lock;
u32 reg_config;
+ enum ina3221_modes mode;
};
static inline bool ina3221_is_enabled(struct ina3221_data *ina, int channel)
@@ -188,6 +196,11 @@ static int ina3221_read_in(struct device *dev, u32 attr,
int channel, long *val)
if (!ina3221_is_enabled(ina, channel))
return -ENODATA;
+ /* Write CONFIG register to trigger a single-shot measurement */
+ if (ina->mode == INA3221_MODE_SINGLE_SHOT)
+ regmap_write(ina->regmap, INA3221_CONFIG,
+ina->reg_config);
+
ret = ina3221_wait_for_data(ina);
if (ret)
return ret;
@@ -232,6 +245,11 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
if (!ina3221_is_enabled(ina, channel))
return -ENODATA;
+ /* Write CONFIG register to trigger a single-shot measurement */
+ if (ina->mode == INA3221_MODE_SINGLE_SHOT)
+ regmap_write(ina->regmap, INA3221_CONFIG,
+ina->reg_config);
+
ret = ina3221_wait_for_data(ina);
if (ret)
return ret;
@@ -617,6 +635,9 @@ static int ina3221_probe_from_dt(struct device *dev, struct
ina3221_data *ina)
if (!np)
return 0;
+ if (of_property_read_bool(np, "ti,single-shot"))
+ ina->mode = INA3221_MODE_SINGLE_SHOT;
+
for_each_child_of_node(np, child) {
ret = ina3221_probe_child_from_dt(dev, child, ina);
if (ret)
@@ -654,6 +675,9 @@ static int ina3221_probe(struct i2c_client *client,
}
}
+ /* Hardware default uses the continuous mode */
+ ina->mode = INA3221_MODE_CONTINUOUS;
+
for (i = 0; i < INA3221_NUM_CHANNELS; i++)
ina->inputs[i].shunt_resistor = INA3221_RSHUNT_DEFAULT;
@@ -666,6 +690,10 @@ static int ina3221_probe(struct i2c_client *client,
/* The driver will be reset, so use reset value */
ina->reg_config = INA3221_CONFIG_DEFAULT;
+ /* Clear continuous bit to use single-shot mode */
+ if (ina->mode == INA3221_MODE_SINGLE_SHOT)
+ ina->reg_config &= ~INA3221_CONFIG_MODE_CONTINUOUS;
+
/* Disable channels if their inputs are disconnected */
for (i = 0; i < INA3221_NUM_CHANNELS; i++) {
if (ina->inputs[i].disconnected)
--
2.17.1
[PATCH 0/2] hwmon (ina3221) Add single-shot mode support
By default, ina3221, as a hardware monitor, continuously measures the inputs and generates corresponding data. However, for battery powered devices, this mode might be power consuming. This series of patches add a feature of changing default operating mode to its single-shot mode via DT. Nicolin Chen (2): dt-bindings: hwmon: ina3221: Add ti,single-shot property hwmon: (ina3221) Implement ti,single-shot DT property .../devicetree/bindings/hwmon/ina3221.txt | 10 +++ drivers/hwmon/ina3221.c | 28 +++ 2 files changed, 38 insertions(+) -- 2.17.1
Re: [RFC][PATCH] hwmon: (ina2xx) Improve current and power reading precision
(Removing "m.pur...@samsung.com" since it's not reachable any more) Hi Stefan, Thank you for the comments. On Wed, Nov 21, 2018 at 04:13:01PM +, Brüns, Stefan wrote: > > === Problem === > > Both methods simplify software routine by fixing one factor, which > > sacrifices the precision of the hardware measurement results. > > > > Using ina226 for example, with method A, the current scale was 1mA > > and the power scale was 25mA. > > > > With method B, calibration value is fixed at 2048 so the precision > > is decided by shunt resistor value. It sounds reasonable since the > > hardware engineers can use a larger shunt resistor when they need > > higher resolution. However, they often concern power burning across > > the resistor as well, so the resistor usually won't be so large: a > > typical value 1000 micro-ohms, which results in a current scale at > > 2.5 mA and a power sacle at 62.5 mW. > > Power loss surely is a concern, but figures should be kept reasonable. > > 1. You mention 1.8V bus voltage, and currents in the 30mA range. Using the > 1mOhm current shunt: > > U_S = R_S * I_S 1e-3 Ohm * 30e-3 A = 30e-6 V (30uV) > P_S = U_S * I_S = 30e-6V * 30e-3 A = 900e-9W = 0.9 uW > > INA219 Power Supply (Datasheet) > Min operating Voltage: 3V > Quiescent Current: 0.7mA > -> Min power: 2.1mW > > So the INA219 alone uses 2.1mW, 1000 times more than the shunt. Chip can enter power-down or one-shot mode. Though this upstream driver doesn't have these two mode supports yet, I am working on it so they'll be added. > Another concern may be voltage drop over the shunt, but for this case you > have > a nominal voltage of 1.8V, so 30uV are 0.001%. > > > When measuring a 1.8v voltage running a small current (e.g. 33 mA), > > the power value (that's supposed to be 59.4 mW) becomes inaccurate > > due to the larger scale (25mA for method A; 62.5 mA for method B). Just found out that I have typos here: 25mW and 62.5mW. > Another look into the datasheet reveals, even at full gain (PGA=1), the LSB is > 40mV / 2^12 = 40mV / 4096 ~ 10uV. So when the current ADC reads out as 3*LSB, > this anything between 25mA and 35mA. This is the best case figure. Current read doesn't get affected a lot actually, since hwmon ABI also reports current value in unit mA. However, the power read is the matter here. With a 62.5mW power_lsb, power results are kinda useless on my system. > On top of quantisation error, you have the ADC offset voltage (V_OS), which > is > given as (for PGA=1, best case): (+-) 10uV typical, (+-) 100uV max. > > So, if you want to have meaningful readouts adjust your shunt to a reasonable > value. Even 100 times the current value will have no measurable effect on you > system (power loss: 90uW, voltage drop: 3mV). I understand your last point. But I believe that'd be a separate idea to improve precision but not an alternative one from driver aspect. Yes, I agree using a larger shunt resistor is always one way to get precision, but it doesn't give an excuse for software driver to be less optimized. The truth is that calibration value is set to the minimum value, giving the best range of current measurement meanwhile the worst precision. You can argue that our hardware engineers might have been too conservative by putting a not-big-enough resistor. But they may also argue that software driver doesn't optimize while the register is totally configurable. I believe both sides have their own reasons. But apparently configuring in software is fairly less expensive. And it doesn't sound convincing to me by telling them "hey guys, go to change your hardware design because we will not set that register": We have more than thousands of products in the market (all battery powered so being conservative), so not possible to recall all of them back just to place larger shunt resistors. >From my point of view, there's nothing wrong for hardware folks being conservative to put a smaller resistor since the software has the capability to improve precision with a single I2C write. A separate topic, I actually thought about setting the default calibration value using a number somewhere in the middle of the best range and the best precision. But it looks that I am right to keep the default to minimum value so that no existing users will be affected. So my patch merely gives an opportunity for those conservative designers to fine-tune the software for a better precision. It is necessary since there are such designers/users and products. And it isn't supposed to affect existing users. Thank you Nicolin
[RFC][PATCH] hwmon: (ina2xx) Improve current and power reading precision
=== Background ===
The ina2xx chip has a CALIBRATION register that's used to configure
calibration value to get the scale of current and power measurement
results from their registers. It basically follows two equations:
1) CAL = calibration factor / (current_lsb_A * rshunt_Ohm)
[ calibration factor: 0.04096 for ina219; 0.00512 for ina226 ]
2) current_lsb_A = max_curr_A / 2^15
According to these two equations, calibration value, current scale,
max_curr_A, and shunt resistor value are related factors.
The driver has been using two methods for these two equations:
A) Used a fixed current_lsb at 1 mA scale
B) Using a fixed calibration value at 4096|2048 for ina219|226
=== Problem ===
Both methods simplify software routine by fixing one factor, which
sacrifices the precision of the hardware measurement results.
Using ina226 for example, with method A, the current scale was 1mA
and the power scale was 25mA.
With method B, calibration value is fixed at 2048 so the precision
is decided by shunt resistor value. It sounds reasonable since the
hardware engineers can use a larger shunt resistor when they need
higher resolution. However, they often concern power burning across
the resistor as well, so the resistor usually won't be so large: a
typical value 1000 micro-ohms, which results in a current scale at
2.5 mA and a power sacle at 62.5 mW.
When measuring a 1.8v voltage running a small current (e.g. 33 mA),
the power value (that's supposed to be 59.4 mW) becomes inaccurate
due to the larger scale (25mA for method A; 62.5 mA for method B).
Also, worth mentioning: method B was added by commit 5d389b125186
("hwmon: (ina2xx) Make calibration register value fixed") stating
that 4096|2048 is the best value for ina219|in226. However, their
datasheets do not seem to mention this "best" keyword. Actually,
they are merely minimum calibration values. See below calculation:
Equation 1, mentioned at Background section:
CAL = calibration factor / (current_lsb_A * rshunt_Ohm)
=> min CAL = calibration factor / (max current_lsb_A * rshunt_Ohm)
[calibration factor is 0.00512 for ina226]
Equation 2, mentioned at Background section:
current_lsb_A = max_curr_A / 2^15
=> max current_lsb_A = (upper bound of max_curr_A) / 2^15
Equation Aux:
(upper bound of max_curr_A) = max Vshunt / rshunt_Ohm
[max Vshunt is 81.92 mA mentioned in ina226 datasheet]
Combining three equations:
min CAL = 0.00512 / (0.08192 / rshunt_Ohm / 2^15 * rshunt_Ohm)
= 0.00512 / (0.08192 / 2^15)
= 2048
=== Solution ===
Being implied in the previous two paragraphs, both methods result
in inaccurate measurements due to the large lsb values because of
the fixed factors for the calculations.
To address this problem, this patch introduces a sysfs attribute
"max_curr" (maximum expected current_lsb_mA) to allow user to set
the current_lsb_uA via this max_curr according to Equation 2, for
precision based on the use case, so they'll be able to indirectly
control the calibration_value through Equation 1.
(In order not to break existing platforms, this change continues
using 4096|2048 as a default calibration value but renames them
from "best" to "min")
Then the change introduces a pair of macros for Equation 1 and 2
and adds the calibration_factor back for Equation 1.
=== Testing ===
Fixed factors:
shunt_resistor = 1000 uO
in1_input = 1799 mV
Before:
max_curr = 81920 mA
calibration_value = 2048
when curr1_input = 33 mA,
power1_input = 125000 uW (expected 59367 uW)
when curr1_input = 35 mA
power1_input = 125000 uW (expected 62965 uW)
After:
set "max_curr" to 5145 mA (smallest within the valid range)
calibration_value = 32611
when curr1_input = 32 mA
power1_input = 58875 uW (expected 57568 uW)
when curr1_input = 35 mA
power1_input = 62800 uW (expected 62965 uW)
=== Summary ===
- "shunt_resistor" attribute sets rshunt and "max_curr" boundaries
- "max_curr" sysfs attribute decides current_lsb_uA and power_lsb_uW
- ina2xx_init() still uses 4096|2048 as default calibration setting
* All of them will calibrate as the factor of Equation 1 is changed
Signed-off-by: Nicolin Chen
---
Note:
This change is rebased upon the following Documentation change:
Documentation: hwmon: Add descriptions for ina2xx sysfs entries
which was sent to the mail-list and is now under review already.
Documentation/hwmon/ina2xx | 2 +
drivers/hwmon/ina2xx.c | 233 ++---
2 files changed, 192 insertions(+), 43 deletions(-)
diff --git a/Documentation/hwmon/ina2xx b/Documentation/hwmon/ina2xx
index 0f36c021192d..ffe53f0f15e1 100644
--- a/Documentation/hwmon/ina2xx
+++ b/Documentation/hwmon/ina2xx
@@ -71,6 +71,8 @@ in1_input Bus voltage(mV) channel
curr1_inputCurrent(mA) measurement channel
power1_input Power(uW) measurement channel
shunt_
Re: [PATCH] hwmon (ina3221) Add single-shot mode support
On Mon, Nov 19, 2018 at 09:45:59AM -0800, Guenter Roeck wrote: > > In short, other than exposing it via a generic ABI to the user > > space, how about defining some policy to maintaining it within > > the driver? > I think that would be a bad idea. It changes timing for everyone > curently using the driver. It also effectively disables monitoring, > which is the main purpose for using this chip (and other hardware > monitoring chips). This is indeed a key difference between iio > and hwmon - the main purpose of chips in the iio subsystem is to > be able report data efficiently to user space, not hardware monitoring. > I do not think it is appropriate to use iio requirements as argument > to change hwmon driver behavior (and vice versa). OK...what about setting a default mode via DT? I didn't expect it to be possible until I found this existing solution. Though it is still in an iio driver, I don't think this should be iio specific. commit 023e30fb0d3a3b9d6b8dc9e47590aa544d58a22f Author: Adriana Reus Date: Tue Nov 24 12:59:49 2015 +0200 Documentation: devicetree: Add property for controlling power saving mode for the us5182 als sensor Add a property to allow changing the default power-saving mode. By default, at read raw the chip will activate and provide one measurent, then it will shut itself down. However, the chip can also work in "continuous" mode which may be more reliable but is also more power consuming. Signed-off-by: Adriana Reus Acked-by: Rob Herring Signed-off-by: Jonathan Cameron Would it be possible for me to apply a similar one? Since hwmon driver uses continuous mode by default, I will add a "one-shot" property instead of "continuous" -- all existing users won't be effected unless they place one-shot properties in DT bindings. Thanks Nicolin
[PATCH] Documentation: hwmon: Add descriptions for ina2xx sysfs entries
There are a few sysfs entries being exposed to user space by the ina2xx hwmon driver while not getting explicitly documented. So this patch just adds a description section for them. Signed-off-by: Nicolin Chen --- Documentation/hwmon/ina2xx | 15 +++ 1 file changed, 15 insertions(+) diff --git a/Documentation/hwmon/ina2xx b/Documentation/hwmon/ina2xx index b8df81f6d6bc..0f36c021192d 100644 --- a/Documentation/hwmon/ina2xx +++ b/Documentation/hwmon/ina2xx @@ -62,3 +62,18 @@ bus and shunt voltage conversion times multiplied by the averaging rate. We don't touch the conversion times and only modify the number of averages. The lower limit of the update_interval is 2 ms, the upper limit is 2253 ms. The actual programmed interval may vary from the desired value. + +General sysfs entries +- + +in0_input Shunt voltage(mV) channel +in1_input Bus voltage(mV) channel +curr1_inputCurrent(mA) measurement channel +power1_input Power(uW) measurement channel +shunt_resistor Shunt resistance(uOhm) channel + +Sysfs entries for ina226, ina230 and ina231 only +- + +update_intervaldata conversion time; affects number of samples used + to average results for shunt and bus voltages. -- 2.17.1
Re: [PATCH] hwmon (ina3221) Add single-shot mode support
Hello Guenter,
On Wed, Nov 14, 2018 at 09:23:30AM -0800, Guenter Roeck wrote:
> > An alternative way (without the sysfs node), after looking at
> > other hwmon code, could be to have a timed polling thread and
> > read data using an update_interval value from ABI. This might
> > turn out to be more complicated as it'll also involve settings
> > of hardware averaging and conversion time. Above all, I cannot
> > figure out a good threshold of update_interval to switch modes.
> >
> update_interval should only be used if it can be configured
> into hardware, not to trigger a polling thread. It should only
> be used in the driver to determine caching intervals.
I see..so it's more like the conversion time settings instead.
> > If you can give some advice of a better implementation, that'd
> > be great.
> >
> From your description, the only real feasible solution would be a
> generic one, with a well defined interface either to userspace or
> to the kernel. The best I can think of would be an in-kernel API
> setting the power operational mode via callbacks. Alternative would
> be a generic ability to set power operational mode from userspace,
> using an ABI which applies to all drivers, not just one.
Hmm. That would make the situation really complicated, I could
understand your concern though.
I searched a little and found that, while the initial ina3221
hwmon driver was under review, Laxman submitted an IIO version
where Jonathan had a similar comments against its "mode" node:
https://www.spinics.net/lists/linux-hwmon/msg00219.html
Quote from his comments {
* There is a lot of ABI in here concerned with oneshot vs continuous.
This seems to me to be more than it should be. We wouldn't expect to
see stuff changing as a result of switching between these modes other
than wrt to when the data shows up. So I'd expect to not see this
directly exposed at all - but rather sit in oneshot unless either:
1) Buffered mode is running (not currently supported)
2) Alerts are on - which I think requires it to be in continuous mode.
}
Since hwmon driver doesn't support buffered mode, what do you
think about having the chip run in single-shot mode by default
but changing it if alerts are on? Though the driver also needs
to support to enable warning/critical alert pins.
In short, other than exposing it via a generic ABI to the user
space, how about defining some policy to maintaining it within
the driver?
Thanks
Nicolin
[PATCH] hwmon: (ina2xx) Fix current value calculation
The current register (04h) has a sign bit at MSB. The comments for this calculation also mention that it's a signed register. However, the regval is unsigned type so result of calculation turns out to be an incorrect value when current is negative. This patch simply fixes this by adding a casting to s16. Signed-off-by: Nicolin Chen --- drivers/hwmon/ina2xx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c index c2252cf452f5..07ee19573b3f 100644 --- a/drivers/hwmon/ina2xx.c +++ b/drivers/hwmon/ina2xx.c @@ -274,7 +274,7 @@ static int ina2xx_get_value(struct ina2xx_data *data, u8 reg, break; case INA2XX_CURRENT: /* signed register, result in mA */ - val = regval * data->current_lsb_uA; + val = (s16)regval * data->current_lsb_uA; val = DIV_ROUND_CLOSEST(val, 1000); break; case INA2XX_CALIBRATION: -- 2.17.1
Re: [PATCH] hwmon (ina3221) Add single-shot mode support
Hi Guenter, On Tue, Nov 13, 2018 at 09:21:02AM -0800, Guenter Roeck wrote: > > > > INA3221 supports both continuous and single-shot modes. When > > > > running in the continuous mode, it keeps measuring the inputs > > > > and converting them to the data register even if there are no > > > > users reading the data out. In this use case, this could be a > > > > power waste. > > > > > > > > So this patch adds a single-shot mode support so that ina3221 > > > > could do measurement and conversion only if users trigger it, > > > > depending on the use case where it only needs to poll data in > > > > a lower frequency. > > > > > > > > The change also exposes "mode" and "available_modes" nodes to > > > > allow users to switch between two operating modes. > > > > > > > Lots and lots of complexity for little gain. Sorry, I don't see > > > the point of this change. > > > > The chip is causing considerable power waste on battery-powered > > devices so we typically use it running in the single-shot mode. > > And you need to be able to do that with a sysfs attribute ? > Are you planning to have some code switching back and forth > between the modes ? > > You'll need to provide a good rationale why this needs to be > runtime configurable. Honestly, our old downstream driver didn't expose it via sysfs. Instead, it had a built-in "governor" to switch modes based on the CPU hotplug state and cpufreq. However, the interface used to register a CPU hotplug notification was already deprecated. And I don't feel this governor is generic enough to be present in the mainline code. For me, it's not that necessary to be a sysfs attribute. I try to add it merely because I cannot find a good criteria for the mode switching in a hwmon driver. So having an open sysfs node may allow user space power daemon to decide its operating mode, since it knows which power mode the system is running at: full speed (charging/charged) or power saving (on-battery), and it knows how often this exact service will poll the sensor data. An alternative way (without the sysfs node), after looking at other hwmon code, could be to have a timed polling thread and read data using an update_interval value from ABI. This might turn out to be more complicated as it'll also involve settings of hardware averaging and conversion time. Above all, I cannot figure out a good threshold of update_interval to switch modes. If you can give some advice of a better implementation, that'd be great. Thanks Nicolin
Re: [PATCH] hwmon (ina3221) Add single-shot mode support
Hi Guenter, On Mon, Nov 12, 2018 at 08:32:48PM -0800, Guenter Roeck wrote: > On Mon, Nov 12, 2018 at 08:23:53PM -0800, Nicolin Chen wrote: > > INA3221 supports both continuous and single-shot modes. When > > running in the continuous mode, it keeps measuring the inputs > > and converting them to the data register even if there are no > > users reading the data out. In this use case, this could be a > > power waste. > > > > So this patch adds a single-shot mode support so that ina3221 > > could do measurement and conversion only if users trigger it, > > depending on the use case where it only needs to poll data in > > a lower frequency. > > > > The change also exposes "mode" and "available_modes" nodes to > > allow users to switch between two operating modes. > > > Lots and lots of complexity for little gain. Sorry, I don't see > the point of this change. The chip is causing considerable power waste on battery-powered devices so we typically use it running in the single-shot mode. Although the chip now can be powered down, but we still need to occasionally poll it for power measurement and critical alerts, so single-shot mode is the best choice for us, considering that the power-down-and-up routine would be way heavier. I could understand that you don't really like it, but it's some feature that we truly need. Do you have any suggestion to write the code that can make it more convincing to you? Thanks Nicolin
Re: [PATCH] hwmon (ina3221) Add mutex lock to shunt nodes
On Mon, Nov 12, 2018 at 08:31:22PM -0800, Guenter Roeck wrote: > On Mon, Nov 12, 2018 at 08:23:24PM -0800, Nicolin Chen wrote: > > The shunt resistor values are used to calculate shunt voltages > > and currents. As a part of sysfs nodes, it would be better to > > get protected with the same mutex too as other sysfs ABI nodes, > > although this is not very critical because the mutex was added > > to mainly protect register access. > > > > So this patch adds the mutex lock to protect the shunt node. > > > > I am missing something here. I don't see the point of this mutex. > It just reads a variable and reports the result. When setting, > it just writes a value. Protecting the conversion of the passed > value with a mutex is pointless. Protecting writing the value > against reading it is just as pointless. The penalty here is that the resistor value would be mismatched between here and the ongoing calculation when a race happens to the sysfs accesses. But I feel it might not be really that hurt to allow it happen. Let's just drop it then: the mutex was used to protect register read/writes any way. Thanks Nicolin
[PATCH] hwmon (lm63) Do not overwrite data->kind
According to the code right before the removed line, data->kind should be either from DT or from id pointer. So there shouldn't be an additional overwriting after the if-else statement. So this patch just removes the overwriting line. Signed-off-by: Nicolin Chen --- Guenter, I have not really tested this change but still sent it out since it looks obvious. Please feel free to drop it if you have concern at an untested change. Thanks. drivers/hwmon/lm63.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c index 4c1770920d29..eac54b9cdeec 100644 --- a/drivers/hwmon/lm63.c +++ b/drivers/hwmon/lm63.c @@ -1120,7 +1120,6 @@ static int lm63_probe(struct i2c_client *client, data->kind = (enum chips)of_device_get_match_data(>dev); else data->kind = id->driver_data; - data->kind = id->driver_data; if (data->kind == lm64) data->temp2_offset = 16000; -- 2.17.1
[PATCH] hwmon (ina3221) Add single-shot mode support
INA3221 supports both continuous and single-shot modes. When
running in the continuous mode, it keeps measuring the inputs
and converting them to the data register even if there are no
users reading the data out. In this use case, this could be a
power waste.
So this patch adds a single-shot mode support so that ina3221
could do measurement and conversion only if users trigger it,
depending on the use case where it only needs to poll data in
a lower frequency.
The change also exposes "mode" and "available_modes" nodes to
allow users to switch between two operating modes.
Signed-off-by: Nicolin Chen
---
Documentation/hwmon/ina3221 | 3 +
drivers/hwmon/ina3221.c | 109
2 files changed, 112 insertions(+)
diff --git a/Documentation/hwmon/ina3221 b/Documentation/hwmon/ina3221
index 4b82cbfb551c..b03f4ad901ee 100644
--- a/Documentation/hwmon/ina3221
+++ b/Documentation/hwmon/ina3221
@@ -35,3 +35,6 @@ curr[123]_max Warning alert current(mA) setting,
activates the
average is above this value.
curr[123]_max_alarm Warning alert current limit exceeded
in[456]_input Shunt voltage(uV) for channels 1, 2, and 3 respectively
+available_modes Available operating modes of the chip:
+ continuous mode; single-shot mode
+modeCurrent operating mode of the chip
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 17a57dbc0424..8f7da3f75d53 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -91,6 +91,17 @@ enum ina3221_channels {
INA3221_NUM_CHANNELS
};
+enum ina3221_modes {
+ INA3221_MODE_SINGLE_SHOT,
+ INA3221_MODE_CONTINUOUS,
+ INA3221_NUM_MODES,
+};
+
+static const char * const ina3221_mode_names[] = {
+ [INA3221_MODE_SINGLE_SHOT] = "single-shot",
+ [INA3221_MODE_CONTINUOUS] = "continuous",
+};
+
/**
* struct ina3221_input - channel input source specific information
* @label: label of channel input source
@@ -127,6 +138,11 @@ static inline bool ina3221_is_enabled(struct ina3221_data
*ina, int channel)
(ina->reg_config & INA3221_CONFIG_CHx_EN(channel));
}
+static inline bool ina3221_is_singleshot_mode(struct ina3221_data *ina)
+{
+ return !(ina->reg_config & INA3221_CONFIG_MODE_CONTINUOUS);
+}
+
/* Lookup table for Bus and Shunt conversion times in usec */
static const u16 ina3221_conv_time[] = {
140, 204, 332, 588, 1100, 2116, 4156, 8244,
@@ -188,6 +204,11 @@ static int ina3221_read_in(struct device *dev, u32 attr,
int channel, long *val)
if (!ina3221_is_enabled(ina, channel))
return -ENODATA;
+ /* Write CONFIG register to trigger a single-shot measurement */
+ if (ina3221_is_singleshot_mode(ina))
+ regmap_write(ina->regmap, INA3221_CONFIG,
+ina->reg_config);
+
ret = ina3221_wait_for_data(ina);
if (ret)
return ret;
@@ -232,6 +253,11 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
if (!ina3221_is_enabled(ina, channel))
return -ENODATA;
+ /* Write CONFIG register to trigger a single-shot measurement */
+ if (ina3221_is_singleshot_mode(ina))
+ regmap_write(ina->regmap, INA3221_CONFIG,
+ina->reg_config);
+
ret = ina3221_wait_for_data(ina);
if (ret)
return ret;
@@ -532,6 +558,82 @@ static ssize_t ina3221_set_shunt(struct device *dev,
return count;
}
+static ssize_t available_modes_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(ina3221_mode_names); i++)
+ snprintf(buf, PAGE_SIZE, "%s%s ", buf, ina3221_mode_names[i]);
+
+ return snprintf(buf, PAGE_SIZE, "%s\n", buf);
+}
+
+static ssize_t mode_show(struct device *dev,
+struct device_attribute *attr, char *buf)
+{
+ struct ina3221_data *ina = dev_get_drvdata(dev);
+ int mode;
+
+ if (ina->reg_config & INA3221_CONFIG_MODE_CONTINUOUS)
+ mode = INA3221_MODE_CONTINUOUS;
+ else
+ mode = INA3221_MODE_SINGLE_SHOT;
+
+ return snprintf(buf, PAGE_SIZE, "%s\n", ina3221_mode_names[mode]);
+}
+
+static ssize_t mode_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct ina3221_data *ina = dev_get_drvdata(dev);
+ u16 mask = INA3221_CONFIG_MODE_CONTINUOUS;
+ u16 continuous;
+ int mode, ret;
+ char na
[PATCH] hwmon (ina3221) Add mutex lock to shunt nodes
The shunt resistor values are used to calculate shunt voltages
and currents. As a part of sysfs nodes, it would be better to
get protected with the same mutex too as other sysfs ABI nodes,
although this is not very critical because the mutex was added
to mainly protect register access.
So this patch adds the mutex lock to protect the shunt node.
Signed-off-by: Nicolin Chen
---
drivers/hwmon/ina3221.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 86281afd2619..f7a09ab6c440 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -532,8 +532,15 @@ static ssize_t ina3221_show_shunt(struct device *dev,
struct ina3221_data *ina = dev_get_drvdata(dev);
unsigned int channel = sd_attr->index;
struct ina3221_input *input = >inputs[channel];
+ ssize_t ret;
- return snprintf(buf, PAGE_SIZE, "%d\n", input->shunt_resistor);
+ mutex_lock(>lock);
+
+ ret = snprintf(buf, PAGE_SIZE, "%d\n", input->shunt_resistor);
+
+ mutex_unlock(>lock);
+
+ return ret;
}
static ssize_t ina3221_set_shunt(struct device *dev,
@@ -547,14 +554,20 @@ static ssize_t ina3221_set_shunt(struct device *dev,
int val;
int ret;
+ mutex_lock(>lock);
+
ret = kstrtoint(buf, 0, );
- if (ret)
+ if (ret) {
+ mutex_unlock(>lock);
return ret;
+ }
val = clamp_val(val, 1, INT_MAX);
input->shunt_resistor = val;
+ mutex_unlock(>lock);
+
return count;
}
--
2.17.1
[PATCH] hwmon (ina2xx) Fix NULL id pointer in probe()
When using DT configurations, the id pointer might turn out to
be NULL. Then the driver encounters NULL pointer access:
Unable to handle kernel read from unreadable memory at vaddr 0018
[...]
PC is at ina2xx_probe+0x114/0x200
LR is at ina2xx_probe+0x10c/0x200
[...]
Kernel panic - not syncing: Attempted to kill init! exitcode=0x000b
The reason is that i2c core returns the id pointer by matching
id_table with client->name, while the client->name is actually
using the name from the first string in the DT compatible list,
not the best one. So i2c core would fail to match the id_table
if the best matched compatible string isn't the first one, and
then would return a NULL id pointer.
This probably should be fixed in i2c core. But it doesn't hurt
to make the driver robust. So this patch fixes it by using the
"chip" that's added to unify both DT and non-DT configurations.
Additionally, since id pointer could be null, so as id->name:
ina2xx 10-0047: power monitor (null) (Rshunt = 1000 uOhm)
ina2xx 10-0048: power monitor (null) (Rshunt = 1 uOhm)
So this patch also fixes NULL name pointer, using client->name
to play safe and to align with hwmon->name.
Fixes: bd0ddd4d0883 ("hwmon: (ina2xx) Add OF device ID table")
Signed-off-by: Nicolin Chen
---
drivers/hwmon/ina2xx.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 71d3445ba869..c2252cf452f5 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -491,7 +491,7 @@ static int ina2xx_probe(struct i2c_client *client,
}
data->groups[group++] = _group;
- if (id->driver_data == ina226)
+ if (chip == ina226)
data->groups[group++] = _group;
hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
@@ -500,7 +500,7 @@ static int ina2xx_probe(struct i2c_client *client,
return PTR_ERR(hwmon_dev);
dev_info(dev, "power monitor %s (Rshunt = %li uOhm)\n",
-id->name, data->rshunt);
+client->name, data->rshunt);
return 0;
}
--
2.17.1
[PATCH v5 1/4] hwmon: (ina3221) Check channel status for alarms attribute read
There is nothing critically wrong to read these two attributes
without having a is_enabled() check at this point. But reading
the MASK_ENABLE register would clear the CVRF bit according to
the datasheet. So it'd be safer to fence for disabled channels
in order to add pm runtime feature.
Signed-off-by: Nicolin Chen
---
Changelog
v2->v5:
* N/A
v1->v2:
* Returned 0 for alert flags instead of -ENODATA
drivers/hwmon/ina3221.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index d61688f04594..26cdf3342d80 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -200,6 +200,12 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
return 0;
case hwmon_curr_crit_alarm:
case hwmon_curr_max_alarm:
+ /* No actual register read if channel is disabled */
+ if (!ina3221_is_enabled(ina, channel)) {
+ /* Return 0 for alert flags */
+ *val = 0;
+ return 0;
+ }
ret = regmap_field_read(ina->fields[reg], );
if (ret)
return ret;
--
2.17.1
[PATCH v5 3/4] hwmon: (ina3221) Make sure data is ready before reading
The data might need some time to get ready after channel enabling,
although the data register is always readable. The CVRF bit is to
indicate that data conversion is finished, so polling the CVRF bit
before data reading could ensure the result being valid.
An alternative way could be to wait for expected time between the
channel enabling and the data reading. And this could avoid extra
I2C communications. However, INA3221 seemly takes longer time than
what's stated in the datasheet. Test results show that sometimes
it couldn't finish data conversion in time.
So this patch plays safe by adding a CVRF polling to make sure the
data register is updated with the new data.
Signed-off-by: Nicolin Chen
---
Changelog
v3->v5:
* N/A
v2->v3:
* Dropped timeout dev_err messages as it's indicated in the errno
v1->v2:
* Moved CVRF polling to data read routine
* Added calculation of wait time based on conversion time setting
drivers/hwmon/ina3221.c | 42 +
1 file changed, 42 insertions(+)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 10e8347a3c80..07dd6ef58d3e 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -44,6 +44,13 @@
#define INA3221_CONFIG_MODE_SHUNT BIT(0)
#define INA3221_CONFIG_MODE_BUSBIT(1)
#define INA3221_CONFIG_MODE_CONTINUOUS BIT(2)
+#define INA3221_CONFIG_VSH_CT_SHIFT3
+#define INA3221_CONFIG_VSH_CT_MASK GENMASK(5, 3)
+#define INA3221_CONFIG_VSH_CT(x) (((x) & GENMASK(5, 3)) >> 3)
+#define INA3221_CONFIG_VBUS_CT_SHIFT 6
+#define INA3221_CONFIG_VBUS_CT_MASKGENMASK(8, 6)
+#define INA3221_CONFIG_VBUS_CT(x) (((x) & GENMASK(8, 6)) >> 6)
+#define INA3221_CONFIG_CHs_EN_MASK GENMASK(14, 12)
#define INA3221_CONFIG_CHx_EN(x) BIT(14 - (x))
#define INA3221_RSHUNT_DEFAULT 1
@@ -52,6 +59,9 @@ enum ina3221_fields {
/* Configuration */
F_RST,
+ /* Status Flags */
+ F_CVRF,
+
/* Alert Flags */
F_WF3, F_WF2, F_WF1,
F_CF3, F_CF2, F_CF1,
@@ -63,6 +73,7 @@ enum ina3221_fields {
static const struct reg_field ina3221_reg_fields[] = {
[F_RST] = REG_FIELD(INA3221_CONFIG, 15, 15),
+ [F_CVRF] = REG_FIELD(INA3221_MASK_ENABLE, 0, 0),
[F_WF3] = REG_FIELD(INA3221_MASK_ENABLE, 3, 3),
[F_WF2] = REG_FIELD(INA3221_MASK_ENABLE, 4, 4),
[F_WF1] = REG_FIELD(INA3221_MASK_ENABLE, 5, 5),
@@ -111,6 +122,28 @@ static inline bool ina3221_is_enabled(struct ina3221_data
*ina, int channel)
return ina->reg_config & INA3221_CONFIG_CHx_EN(channel);
}
+/* Lookup table for Bus and Shunt conversion times in usec */
+static const u16 ina3221_conv_time[] = {
+ 140, 204, 332, 588, 1100, 2116, 4156, 8244,
+};
+
+static inline int ina3221_wait_for_data(struct ina3221_data *ina)
+{
+ 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 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);
+
+ /* Polling the CVRF bit to make sure read data is ready */
+ return regmap_field_read_poll_timeout(ina->fields[F_CVRF],
+ cvrf, cvrf, wait, 10);
+}
+
static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
int *val)
{
@@ -150,6 +183,10 @@ static int ina3221_read_in(struct device *dev, u32 attr,
int channel, long *val)
if (!ina3221_is_enabled(ina, channel))
return -ENODATA;
+ ret = ina3221_wait_for_data(ina);
+ if (ret)
+ return ret;
+
ret = ina3221_read_value(ina, reg, );
if (ret)
return ret;
@@ -189,6 +226,11 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
case hwmon_curr_input:
if (!ina3221_is_enabled(ina, channel))
return -ENODATA;
+
+ ret = ina3221_wait_for_data(ina);
+ if (ret)
+ return ret;
+
/* fall through */
case hwmon_curr_crit:
case hwmon_curr_max:
--
2.17.1
[PATCH v5 2/4] hwmon: (ina3221) Serialize sysfs ABI accesses
This change adds a mutex to serialize accesses of sysfs attributes.
This is required when polling CVRF bit of the MASK/ENABLE register
because this bit is cleared on a read of this MASK/ENABLE register
or a write to CONFIG register, which means that this bit might be
accidentally cleared by reading other fields like alert flags.
So this patch adds a mutex lock to protect the write() and read()
callbacks. The read_string() callback won't need the lock since it
just returns the label without touching any hardware register.
Signed-off-by: Nicolin Chen
---
Changlog
v1->v5:
* N/A
drivers/hwmon/ina3221.c | 51 -
1 file changed, 45 insertions(+), 6 deletions(-)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 26cdf3342d80..10e8347a3c80 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
#include
#include
@@ -94,12 +95,14 @@ struct ina3221_input {
* @regmap: Register map of the device
* @fields: Register fields of the device
* @inputs: Array of channel input source specific structures
+ * @lock: mutex lock to serialize sysfs attribute accesses
* @reg_config: Register value of INA3221_CONFIG
*/
struct ina3221_data {
struct regmap *regmap;
struct regmap_field *fields[F_MAX_FIELDS];
struct ina3221_input inputs[INA3221_NUM_CHANNELS];
+ struct mutex lock;
u32 reg_config;
};
@@ -265,29 +268,53 @@ static int ina3221_write_enable(struct device *dev, int
channel, bool enable)
static int ina3221_read(struct device *dev, enum hwmon_sensor_types type,
u32 attr, int channel, long *val)
{
+ struct ina3221_data *ina = dev_get_drvdata(dev);
+ int ret;
+
+ mutex_lock(>lock);
+
switch (type) {
case hwmon_in:
/* 0-align channel ID */
- return ina3221_read_in(dev, attr, channel - 1, val);
+ ret = ina3221_read_in(dev, attr, channel - 1, val);
+ break;
case hwmon_curr:
- return ina3221_read_curr(dev, attr, channel, val);
+ ret = ina3221_read_curr(dev, attr, channel, val);
+ break;
default:
- return -EOPNOTSUPP;
+ ret = -EOPNOTSUPP;
+ break;
}
+
+ mutex_unlock(>lock);
+
+ return ret;
}
static int ina3221_write(struct device *dev, enum hwmon_sensor_types type,
u32 attr, int channel, long val)
{
+ struct ina3221_data *ina = dev_get_drvdata(dev);
+ int ret;
+
+ mutex_lock(>lock);
+
switch (type) {
case hwmon_in:
/* 0-align channel ID */
- return ina3221_write_enable(dev, channel - 1, val);
+ ret = ina3221_write_enable(dev, channel - 1, val);
+ break;
case hwmon_curr:
- return ina3221_write_curr(dev, attr, channel, val);
+ ret = ina3221_write_curr(dev, attr, channel, val);
+ break;
default:
- return -EOPNOTSUPP;
+ ret = -EOPNOTSUPP;
+ break;
}
+
+ mutex_unlock(>lock);
+
+ return ret;
}
static int ina3221_read_string(struct device *dev, enum hwmon_sensor_types
type,
@@ -582,6 +609,7 @@ static int ina3221_probe(struct i2c_client *client,
if (ret)
return ret;
+ mutex_init(>lock);
dev_set_drvdata(dev, ina);
hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, ina,
@@ -589,12 +617,22 @@ static int ina3221_probe(struct i2c_client *client,
ina3221_groups);
if (IS_ERR(hwmon_dev)) {
dev_err(dev, "Unable to register hwmon device\n");
+ mutex_destroy(>lock);
return PTR_ERR(hwmon_dev);
}
return 0;
}
+static int ina3221_remove(struct i2c_client *client)
+{
+ struct ina3221_data *ina = dev_get_drvdata(>dev);
+
+ mutex_destroy(>lock);
+
+ return 0;
+}
+
static int __maybe_unused ina3221_suspend(struct device *dev)
{
struct ina3221_data *ina = dev_get_drvdata(dev);
@@ -663,6 +701,7 @@ MODULE_DEVICE_TABLE(i2c, ina3221_ids);
static struct i2c_driver ina3221_i2c_driver = {
.probe = ina3221_probe,
+ .remove = ina3221_remove,
.driver = {
.name = INA3221_DRIVER_NAME,
.of_match_table = ina3221_of_match_table,
--
2.17.1
[PATCH v5 4/4] hwmon: (ina3221) Add PM runtime support
If all three channels are disabled via in[123]_enable ABI,
the driver could suspend the chip for power saving purpose.
So this patch adds the PM runtime support in order to gain
more power control than system suspend and resume use case.
For PM runtime, there are a few related changes happening:
1) Added a new pm_dev device pointer for all the PM runtime
callbacks. This is because hwmon core registers a child
device for each hwmon driver and passes it back to each
driver. So there might be a mismatch between two device
pointers in the driver if mixing using them.
2) Added a check in ina3221_is_enabled() to make sure that
the chip is resumed.
3) Bypassed the unchanged status in ina3221_write_enable()
in order to keep the PM runtime refcount being matched.
4) Removed the reset routine in the probe() by calling the
resume() via pm_runtime_get_sync() instead, as they're
similar. It's also necessary to do so to match initial
PM refcount with the number of enabled channels.
Signed-off-by: Nicolin Chen
---
Changelog
v4->v5:
* Dropped the code of passing pm pointer via _info API
** Moved all changes back to the status of v3
* Used i2c_client dev pointer for pm runtime callbacks
v3->v4:
* Passed pm pointer via _with_info API instead the i2c driver
v2->v3:
* Improved a dev_err message
* Added comments at pm_runtime_put_noidle() callbacks
* Added pm_runtime header file in an alphabetical order
v1->v2:
* Bypassed i2c_client->dev in suspend/resume()
* Added a missing '\n' in one dev_err()
drivers/hwmon/ina3221.c | 93 -
1 file changed, 74 insertions(+), 19 deletions(-)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 07dd6ef58d3e..17a57dbc0424 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -20,6 +20,7 @@
#include
#include
#include
+#include
#include
#define INA3221_DRIVER_NAME"ina3221"
@@ -53,6 +54,7 @@
#define INA3221_CONFIG_CHs_EN_MASK GENMASK(14, 12)
#define INA3221_CONFIG_CHx_EN(x) BIT(14 - (x))
+#define INA3221_CONFIG_DEFAULT 0x7127
#define INA3221_RSHUNT_DEFAULT 1
enum ina3221_fields {
@@ -103,6 +105,7 @@ struct ina3221_input {
/**
* struct ina3221_data - device specific information
+ * @pm_dev: Device pointer for pm runtime
* @regmap: Register map of the device
* @fields: Register fields of the device
* @inputs: Array of channel input source specific structures
@@ -110,6 +113,7 @@ struct ina3221_input {
* @reg_config: Register value of INA3221_CONFIG
*/
struct ina3221_data {
+ struct device *pm_dev;
struct regmap *regmap;
struct regmap_field *fields[F_MAX_FIELDS];
struct ina3221_input inputs[INA3221_NUM_CHANNELS];
@@ -119,7 +123,8 @@ struct ina3221_data {
static inline bool ina3221_is_enabled(struct ina3221_data *ina, int channel)
{
- return ina->reg_config & INA3221_CONFIG_CHx_EN(channel);
+ return pm_runtime_active(ina->pm_dev) &&
+ (ina->reg_config & INA3221_CONFIG_CHx_EN(channel));
}
/* Lookup table for Bus and Shunt conversion times in usec */
@@ -290,21 +295,48 @@ static int ina3221_write_enable(struct device *dev, int
channel, bool enable)
{
struct ina3221_data *ina = dev_get_drvdata(dev);
u16 config, mask = INA3221_CONFIG_CHx_EN(channel);
+ u16 config_old = ina->reg_config & mask;
int ret;
config = enable ? mask : 0;
+ /* Bypass if enable status is not being changed */
+ if (config_old == config)
+ return 0;
+
+ /* For enabling routine, increase refcount and resume() at first */
+ if (enable) {
+ ret = pm_runtime_get_sync(ina->pm_dev);
+ if (ret < 0) {
+ dev_err(dev, "Failed to get PM runtime\n");
+ return ret;
+ }
+ }
+
/* Enable or disable the channel */
ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, config);
if (ret)
- return ret;
+ goto fail;
/* Cache the latest config register value */
ret = regmap_read(ina->regmap, INA3221_CONFIG, >reg_config);
if (ret)
- return ret;
+ goto fail;
+
+ /* For disabling routine, decrease refcount or suspend() at last */
+ if (!enable)
+ pm_runtime_put_sync(ina->pm_dev);
return 0;
+
+fail:
+ if (enable) {
+ dev_err(dev, "Failed to enable channel %d: error %d\n",
+ channel, ret);
+ pm_runtime_put_sync(ina->pm_dev);
+ }
+
+ return ret;
}
static int ina3221_read(struct device *dev, enum hwmon_sensor_types type,
@@ -631,44 +663,65 @@ static int ina3221_probe(struct i2c_client *client,
r
[PATCH v5 0/4] hwmon: (ina3221) Implement PM runtime to save power
This series patches implement PM runtime feature in the ina3221 hwmon driver (PATCH-4). However, PATCH-[1:3] are required to make sure that the PM runtime feature would be functional and safe. Changelog v4->v5: * Dropped the change for hwmon core; Shifted all the patch indexes * Used i2c_client dev pointer for pm runtime callbacks (PATCH-4) v3->v4: * Added generic pm runtime functions to hwmon class (PATCH-1) * Updated to pass pm pointer via _with_info API (PATCH-5) v2->v3: * Dropped timeout dev_err messages as it's indicated in errno (PATCH-4) * Improved a dev_err message and comments (PATCH-5) v1->v2: * Added device pointer check (PATCH-1) * Returned 0 for alert flags (PATCH-2) * Moved CVRF polling to data read routine (PATCH-4) * Bypassed i2c_client->dev in suspend/resume() (PATCH-5) Nicolin Chen (4): hwmon: (ina3221) Check channel status for alarms attribute read hwmon: (ina3221) Serialize sysfs ABI accesses hwmon: (ina3221) Make sure data is ready before reading hwmon: (ina3221) Add PM runtime support drivers/hwmon/ina3221.c | 190 +++- 1 file changed, 166 insertions(+), 24 deletions(-) -- 2.17.1
Re: [PATCH v4 1/5] hwmon: (core) Add pm ops to hwmon class
On Fri, Nov 02, 2018 at 10:54:35AM -0700, Guenter Roeck wrote: > > Actually, pm_runtime core also scans a class-level pm pointer: > > else if (dev->class && dev->class->pm) > > ops = dev->class->pm; > > > > This means that hwmon class in the hwmon core could actually > > have its own generic pm callbacks so that a registered driver > > would have the capability to link their own callbacks to the > > hwmon core's. > > > > So this patch adds a pm pointer to the hwmon class with some > > generic pm callbacks of system suspend/resume and pm_runtime > > suspend/resume, and also allows hwmon drivers to pass valid > > pm pointers through _with_info API when registering devices. > > > > Just to give an update: I am not happy with having to add hwmon > specific pm callbacks. That doesn't look right to me. I'll have > to spend time figuring out how other virtual devices handle this > situation. Unfortunately, time is always scarce, so this will likely > take a while. That's okay. Would it be possible then for me to add PM runtime functions just in ina3221, without touching hwmon core? It will belong to the I2C device, not hwmon device any more. > Also, please note that I won't accept function macros. I understand > that are used a lot, especially in older hwmon drivers, but they just > make the code more difficult to read and often add unnecessary code. Okay. Will be careful to use them in future hwmon patches. Thanks Nicolin
[PATCH v4 5/5] hwmon: (ina3221) Add PM runtime support
If all three channels are disabled via in[123]_enable ABI,
the driver could suspend the chip for power saving purpose.
So this patch adds the PM runtime support in order to gain
more power control than system suspend and resume use case.
For PM runtime, there are a few related changes happening:
1) Added a new explicit hdev device pointer for all the PM
runtime callbacks. This is because hwmon core registers
a child device for each hwmon driver. So there might be
a mismatch between two device pointers in the driver if
mixing using them.
2) Passed the pm pointer via _with_info API to hwmon core
so as to let the hwmon class control those callbacks.
3) Added a check in ina3221_is_enabled() to make sure that
the chip is resumed.
4) Bypassed the unchanged status in ina3221_write_enable()
in order to keep the PM runtime refcount being matched.
5) Removed the reset routine in the probe() by calling the
resume() via pm_runtime_get_sync() instead, as they're
similar. It's also necessary to do so to match initial
PM refcount with the number of enabled channels.
Signed-off-by: Nicolin Chen
---
Changelog
v3->v4:
* Passed pm pointer via _with_info API instead the i2c driver
v2->v3:
* Improved a dev_err message
* Added comments at pm_runtime_put_noidle() callbacks
* Added pm_runtime header file in an alphabetical order
v1->v2:
* Bypassed i2c_client->dev in suspend/resume()
* Added a missing '\n' in one dev_err()
drivers/hwmon/ina3221.c | 213 ++--
1 file changed, 137 insertions(+), 76 deletions(-)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 07dd6ef58d3e..974a29b1c8f0 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -20,6 +20,7 @@
#include
#include
#include
+#include
#include
#define INA3221_DRIVER_NAME"ina3221"
@@ -53,6 +54,7 @@
#define INA3221_CONFIG_CHs_EN_MASK GENMASK(14, 12)
#define INA3221_CONFIG_CHx_EN(x) BIT(14 - (x))
+#define INA3221_CONFIG_DEFAULT 0x7127
#define INA3221_RSHUNT_DEFAULT 1
enum ina3221_fields {
@@ -103,6 +105,7 @@ struct ina3221_input {
/**
* struct ina3221_data - device specific information
+ * @hdev: Device pointer of hwmon child device, used for pm runtime
* @regmap: Register map of the device
* @fields: Register fields of the device
* @inputs: Array of channel input source specific structures
@@ -110,6 +113,7 @@ struct ina3221_input {
* @reg_config: Register value of INA3221_CONFIG
*/
struct ina3221_data {
+ struct device *hdev;
struct regmap *regmap;
struct regmap_field *fields[F_MAX_FIELDS];
struct ina3221_input inputs[INA3221_NUM_CHANNELS];
@@ -119,7 +123,8 @@ struct ina3221_data {
static inline bool ina3221_is_enabled(struct ina3221_data *ina, int channel)
{
- return ina->reg_config & INA3221_CONFIG_CHx_EN(channel);
+ return pm_runtime_active(ina->hdev) &&
+ (ina->reg_config & INA3221_CONFIG_CHx_EN(channel));
}
/* Lookup table for Bus and Shunt conversion times in usec */
@@ -290,21 +295,48 @@ static int ina3221_write_enable(struct device *dev, int
channel, bool enable)
{
struct ina3221_data *ina = dev_get_drvdata(dev);
u16 config, mask = INA3221_CONFIG_CHx_EN(channel);
+ u16 config_old = ina->reg_config & mask;
int ret;
config = enable ? mask : 0;
+ /* Bypass if enable status is not being changed */
+ if (config_old == config)
+ return 0;
+
+ /* For enabling routine, increase refcount and resume() at first */
+ if (enable) {
+ ret = pm_runtime_get_sync(ina->hdev);
+ if (ret < 0) {
+ dev_err(dev, "Failed to get PM runtime\n");
+ return ret;
+ }
+ }
+
/* Enable or disable the channel */
ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, config);
if (ret)
- return ret;
+ goto fail;
/* Cache the latest config register value */
ret = regmap_read(ina->regmap, INA3221_CONFIG, >reg_config);
if (ret)
- return ret;
+ goto fail;
+
+ /* For disabling routine, decrease refcount or suspend() at last */
+ if (!enable)
+ pm_runtime_put_sync(ina->hdev);
return 0;
+
+fail:
+ if (enable) {
+ dev_err(dev, "Failed to enable channel %d: error %d\n",
+ channel, ret);
+ pm_runtime_put_sync(ina->hdev);
+ }
+
+ return ret;
}
static int ina3221_read(struct device *dev, enum hwmon_sensor_types type,
@@ -461,9 +493,66 @@ static const struct hwmon_ops ina3221_hwmon_ops = {
.write = ina3221_write,
};
+static int __maybe_unused ina3221_suspend(struct device *dev)
[PATCH v4 3/5] hwmon: (ina3221) Serialize sysfs ABI accesses
This change adds a mutex to serialize accesses of sysfs attributes.
This is required when polling CVRF bit of the MASK/ENABLE register
because this bit is cleared on a read of this MASK/ENABLE register
or a write to CONFIG register, which means that this bit might be
accidentally cleared by reading other fields like alert flags.
So this patch adds a mutex lock to protect the write() and read()
callbacks. The read_string() callback won't need the lock since it
just returns the label without touching any hardware register.
Signed-off-by: Nicolin Chen
---
Changlog
v1->v4:
* N/A
drivers/hwmon/ina3221.c | 51 -
1 file changed, 45 insertions(+), 6 deletions(-)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 26cdf3342d80..10e8347a3c80 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
#include
#include
@@ -94,12 +95,14 @@ struct ina3221_input {
* @regmap: Register map of the device
* @fields: Register fields of the device
* @inputs: Array of channel input source specific structures
+ * @lock: mutex lock to serialize sysfs attribute accesses
* @reg_config: Register value of INA3221_CONFIG
*/
struct ina3221_data {
struct regmap *regmap;
struct regmap_field *fields[F_MAX_FIELDS];
struct ina3221_input inputs[INA3221_NUM_CHANNELS];
+ struct mutex lock;
u32 reg_config;
};
@@ -265,29 +268,53 @@ static int ina3221_write_enable(struct device *dev, int
channel, bool enable)
static int ina3221_read(struct device *dev, enum hwmon_sensor_types type,
u32 attr, int channel, long *val)
{
+ struct ina3221_data *ina = dev_get_drvdata(dev);
+ int ret;
+
+ mutex_lock(>lock);
+
switch (type) {
case hwmon_in:
/* 0-align channel ID */
- return ina3221_read_in(dev, attr, channel - 1, val);
+ ret = ina3221_read_in(dev, attr, channel - 1, val);
+ break;
case hwmon_curr:
- return ina3221_read_curr(dev, attr, channel, val);
+ ret = ina3221_read_curr(dev, attr, channel, val);
+ break;
default:
- return -EOPNOTSUPP;
+ ret = -EOPNOTSUPP;
+ break;
}
+
+ mutex_unlock(>lock);
+
+ return ret;
}
static int ina3221_write(struct device *dev, enum hwmon_sensor_types type,
u32 attr, int channel, long val)
{
+ struct ina3221_data *ina = dev_get_drvdata(dev);
+ int ret;
+
+ mutex_lock(>lock);
+
switch (type) {
case hwmon_in:
/* 0-align channel ID */
- return ina3221_write_enable(dev, channel - 1, val);
+ ret = ina3221_write_enable(dev, channel - 1, val);
+ break;
case hwmon_curr:
- return ina3221_write_curr(dev, attr, channel, val);
+ ret = ina3221_write_curr(dev, attr, channel, val);
+ break;
default:
- return -EOPNOTSUPP;
+ ret = -EOPNOTSUPP;
+ break;
}
+
+ mutex_unlock(>lock);
+
+ return ret;
}
static int ina3221_read_string(struct device *dev, enum hwmon_sensor_types
type,
@@ -582,6 +609,7 @@ static int ina3221_probe(struct i2c_client *client,
if (ret)
return ret;
+ mutex_init(>lock);
dev_set_drvdata(dev, ina);
hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, ina,
@@ -589,12 +617,22 @@ static int ina3221_probe(struct i2c_client *client,
ina3221_groups);
if (IS_ERR(hwmon_dev)) {
dev_err(dev, "Unable to register hwmon device\n");
+ mutex_destroy(>lock);
return PTR_ERR(hwmon_dev);
}
return 0;
}
+static int ina3221_remove(struct i2c_client *client)
+{
+ struct ina3221_data *ina = dev_get_drvdata(>dev);
+
+ mutex_destroy(>lock);
+
+ return 0;
+}
+
static int __maybe_unused ina3221_suspend(struct device *dev)
{
struct ina3221_data *ina = dev_get_drvdata(dev);
@@ -663,6 +701,7 @@ MODULE_DEVICE_TABLE(i2c, ina3221_ids);
static struct i2c_driver ina3221_i2c_driver = {
.probe = ina3221_probe,
+ .remove = ina3221_remove,
.driver = {
.name = INA3221_DRIVER_NAME,
.of_match_table = ina3221_of_match_table,
--
2.17.1
[PATCH v4 0/5] hwmon: (ina3221) Implement PM runtime to save power
This series patches implement PM runtime feature in the ina3221 hwmon driver (PATCH-5). However, PATCH-[1:4] are required to make sure that the PM runtime feature would be functional and safe. Changelog v3->v4: * Added generic pm runtime functions to hwmon class (PATCH-1) * Updated to pass pm pointer via _with_info API (PATCH-5) v2->v3: * Dropped timeout dev_err messages as it's indicated in errno (PATCH-4) * Improved a dev_err message and comments (PATCH-5) v1->v2: * Added device pointer check (PATCH-1) * Returned 0 for alert flags (PATCH-2) * Moved CVRF polling to data read routine (PATCH-4) * Bypassed i2c_client->dev in suspend/resume() (PATCH-5) Nicolin Chen (5): hwmon: (core) Add pm_runtime to hwmon class hwmon: (ina3221) Check channel status for alarms attribute read hwmon: (ina3221) Serialize sysfs ABI accesses hwmon: (ina3221) Make sure data is ready before reading hwmon: (ina3221) Add PM runtime support drivers/hwmon/hwmon.c | 24 drivers/hwmon/ina3221.c | 290 ++-- include/linux/hwmon.h | 2 + 3 files changed, 245 insertions(+), 71 deletions(-) -- 2.17.1
[PATCH v4 1/5] hwmon: (core) Add pm ops to hwmon class
The hwmon core generates an extra child dev pointer for every
registered hwmon driver so as to link the new device to hwmon
class, and it exposes this new dev in /sys/class/hwmon/hwmon*/
directory including a power directory for pm runtime. However,
there is currently no way for hwmon drivers to link their own
pm related information to this power directory, so it's always
showing unsupported even if the driver implements the pm ops.
This is because pm_runtime core scans PM runtime callbacks in
the dev->driver pointer while this new child dev doesn't have
a driver pointer. It sounds easier to merely copy this driver
pointer from the parent dev to the child dev, however, this'd
create some problem like the same suspend() function might be
called twice during system suspend cycle and the second call
may fail since the device is already suspended, so the driver
would have to work around to bypass one of the two callbacks.
Actually, pm_runtime core also scans a class-level pm pointer:
else if (dev->class && dev->class->pm)
ops = dev->class->pm;
This means that hwmon class in the hwmon core could actually
have its own generic pm callbacks so that a registered driver
would have the capability to link their own callbacks to the
hwmon core's.
So this patch adds a pm pointer to the hwmon class with some
generic pm callbacks of system suspend/resume and pm_runtime
suspend/resume, and also allows hwmon drivers to pass valid
pm pointers through _with_info API when registering devices.
Signed-off-by: Nicolin Chen
---
Changelog
v3->v4:
* Dropped the risky pointer copies
* Added generic pm runtime callbacks to the hwmon class
v2->v3:
* N/A
v1->v2:
* Added device pointers
drivers/hwmon/hwmon.c | 24
include/linux/hwmon.h | 2 ++
2 files changed, 26 insertions(+)
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 975c95169884..10bbd36be4a5 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -20,6 +20,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -103,11 +104,34 @@ static void hwmon_dev_release(struct device *dev)
kfree(to_hwmon_device(dev));
}
+#define HWMON_PM_FUNCTION(name)\
+static int __maybe_unused hwmon_##name(struct device *dev) \
+{ \
+ struct hwmon_device *hwdev = to_hwmon_device(dev); \
+ const struct hwmon_chip_info *chip = hwdev->chip; \
+ \
+ if (chip && chip->pm && chip->pm->name) \
+ return chip->pm->name(dev); \
+ \
+ return 0; \
+}
+
+HWMON_PM_FUNCTION(suspend)
+HWMON_PM_FUNCTION(resume)
+HWMON_PM_FUNCTION(runtime_suspend)
+HWMON_PM_FUNCTION(runtime_resume)
+
+static const struct dev_pm_ops hwmon_pm = {
+ SET_SYSTEM_SLEEP_PM_OPS(hwmon_suspend, hwmon_resume)
+ SET_RUNTIME_PM_OPS(hwmon_runtime_suspend, hwmon_runtime_resume, NULL)
+};
+
static struct class hwmon_class = {
.name = "hwmon",
.owner = THIS_MODULE,
.dev_groups = hwmon_dev_attr_groups,
.dev_release = hwmon_dev_release,
+ .pm = _pm,
};
static DEFINE_IDA(hwmon_ida);
diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 99e0c1b0b5fb..edbeddb489f8 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -369,10 +369,12 @@ struct hwmon_channel_info {
* Chip configuration
* @ops: Pointer to hwmon operations.
* @info: Null-terminated list of channel information.
+ * @pm:Pointer to dev_pm_ops callbacks.
*/
struct hwmon_chip_info {
const struct hwmon_ops *ops;
const struct hwmon_channel_info **info;
+ const struct dev_pm_ops *pm;
};
/* hwmon_device_register() is deprecated */
--
2.17.1
Re: [PATCH v3 1/5] hwmon: (core) Inherit power properties to hdev
On Thu, Oct 25, 2018 at 06:55:31AM +, li...@roeck-us.net wrote: > > won't work then. I guess it'd be safer to ignore the problem of > > the power node, i.e. using parent dev pointer for pm runtime. > > > It might be worthwhile looking up how other virtal devices handle > this problem. Maybe the hwmon code could have its own suspend/resume > callbacks. Not sure how to make that work, though, and what those > callbacks would (have to) do. I am adding a core pm pointer to the class. A hwmon driver will also need to pass a private pm pointer in a chip/info structure. I will send it soon. Thanks Nicolin
Re: [PATCH v3 1/5] hwmon: (core) Inherit power properties to hdev
On Wed, Oct 24, 2018 at 06:01:16PM -0700, Nicolin Chen wrote:
> On Thu, Oct 25, 2018 at 12:13:01AM +, li...@roeck-us.net wrote:
>
> > > + if (dev) {
> > > + hdev->driver = dev->driver;
> > > + hdev->power = dev->power;
> > > + hdev->pm_domain = dev->pm_domain;
> > > + hdev->of_node = dev->of_node;
> > > + }
> >
> > We'l need to dig into this more; I suspect it may be inappropriate to do
> > this.
> > With this change, every hwmon driver supporting (runtime ?) suspend/resume
> > will have the problem worked around in #5, and that just seems wrong.
>
> Hmm..that's true...thanks for catching it.
>
> Actually I am not sure the reason of having a child device in
> the core, but could we use the parent dev pointer in the hwmon
> core as hwmon_dev upon confirming parent dev pointer != NULL?
I just noticed that it is used to link to hwmon class. So this
won't work then. I guess it'd be safer to ignore the problem of
the power node, i.e. using parent dev pointer for pm runtime.
Thanks
Nicolin
> The problem here is that the power directory under each hwmon
> directory is tied to the hwmon_dev pointer, not to the parent
> dev pointer, and the hwmon core creates all sysfs nodes based
> on the child node. So those nodes under power directory won't
> be valid unless we copy all pm information, especially PM ops.
>
> There is an option of ignoring this problem though, while all
> hwmon drivers will need to be careful of mixing using the dev
> pointers. So it'd be a lot of easier if we could just use the
> original dev pointer in the core since we mainly just need to
> create sysfs nodes.
>
> Another way of doing this might be to pass down the PM pointer
> via _info structure instead of linking it to the parent driver,
> which then will forbid all hwmon drivers having its own PM ops
> callbacks -- the very opposite way of this patch, and it does
> not sound fully reasonable and feasible to me...
>
> What do you think about?
>
> Thanks
> Nicolin
Re: [PATCH v3 1/5] hwmon: (core) Inherit power properties to hdev
On Thu, Oct 25, 2018 at 12:13:01AM +, li...@roeck-us.net wrote:
> > + if (dev) {
> > + hdev->driver = dev->driver;
> > + hdev->power = dev->power;
> > + hdev->pm_domain = dev->pm_domain;
> > + hdev->of_node = dev->of_node;
> > + }
>
> We'l need to dig into this more; I suspect it may be inappropriate to do this.
> With this change, every hwmon driver supporting (runtime ?) suspend/resume
> will have the problem worked around in #5, and that just seems wrong.
Hmm..that's true...thanks for catching it.
Actually I am not sure the reason of having a child device in
the core, but could we use the parent dev pointer in the hwmon
core as hwmon_dev upon confirming parent dev pointer != NULL?
The problem here is that the power directory under each hwmon
directory is tied to the hwmon_dev pointer, not to the parent
dev pointer, and the hwmon core creates all sysfs nodes based
on the child node. So those nodes under power directory won't
be valid unless we copy all pm information, especially PM ops.
There is an option of ignoring this problem though, while all
hwmon drivers will need to be careful of mixing using the dev
pointers. So it'd be a lot of easier if we could just use the
original dev pointer in the core since we mainly just need to
create sysfs nodes.
Another way of doing this might be to pass down the PM pointer
via _info structure instead of linking it to the parent driver,
which then will forbid all hwmon drivers having its own PM ops
callbacks -- the very opposite way of this patch, and it does
not sound fully reasonable and feasible to me...
What do you think about?
Thanks
Nicolin
[PATCH v3 4/5] hwmon: (ina3221) Make sure data is ready before reading
The data might need some time to get ready after channel enabling,
although the data register is always readable. The CVRF bit is to
indicate that data conversion is finished, so polling the CVRF bit
before data reading could ensure the result being valid.
An alternative way could be to wait for expected time between the
channel enabling and the data reading. And this could avoid extra
I2C communications. However, INA3221 seemly takes longer time than
what's stated in the datasheet. Test results show that sometimes
it couldn't finish data conversion in time.
So this patch plays safe by adding a CVRF polling to make sure the
data register is updated with the new data.
Signed-off-by: Nicolin Chen
---
Changelog
v2->v3:
* Dropped timeout dev_err messages as it's indicated in the errno
v1->v2:
* Moved CVRF polling to data read routine
* Added calculation of wait time based on conversion time setting
drivers/hwmon/ina3221.c | 42 +
1 file changed, 42 insertions(+)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 10e8347a3c80..07dd6ef58d3e 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -44,6 +44,13 @@
#define INA3221_CONFIG_MODE_SHUNT BIT(0)
#define INA3221_CONFIG_MODE_BUSBIT(1)
#define INA3221_CONFIG_MODE_CONTINUOUS BIT(2)
+#define INA3221_CONFIG_VSH_CT_SHIFT3
+#define INA3221_CONFIG_VSH_CT_MASK GENMASK(5, 3)
+#define INA3221_CONFIG_VSH_CT(x) (((x) & GENMASK(5, 3)) >> 3)
+#define INA3221_CONFIG_VBUS_CT_SHIFT 6
+#define INA3221_CONFIG_VBUS_CT_MASKGENMASK(8, 6)
+#define INA3221_CONFIG_VBUS_CT(x) (((x) & GENMASK(8, 6)) >> 6)
+#define INA3221_CONFIG_CHs_EN_MASK GENMASK(14, 12)
#define INA3221_CONFIG_CHx_EN(x) BIT(14 - (x))
#define INA3221_RSHUNT_DEFAULT 1
@@ -52,6 +59,9 @@ enum ina3221_fields {
/* Configuration */
F_RST,
+ /* Status Flags */
+ F_CVRF,
+
/* Alert Flags */
F_WF3, F_WF2, F_WF1,
F_CF3, F_CF2, F_CF1,
@@ -63,6 +73,7 @@ enum ina3221_fields {
static const struct reg_field ina3221_reg_fields[] = {
[F_RST] = REG_FIELD(INA3221_CONFIG, 15, 15),
+ [F_CVRF] = REG_FIELD(INA3221_MASK_ENABLE, 0, 0),
[F_WF3] = REG_FIELD(INA3221_MASK_ENABLE, 3, 3),
[F_WF2] = REG_FIELD(INA3221_MASK_ENABLE, 4, 4),
[F_WF1] = REG_FIELD(INA3221_MASK_ENABLE, 5, 5),
@@ -111,6 +122,28 @@ static inline bool ina3221_is_enabled(struct ina3221_data
*ina, int channel)
return ina->reg_config & INA3221_CONFIG_CHx_EN(channel);
}
+/* Lookup table for Bus and Shunt conversion times in usec */
+static const u16 ina3221_conv_time[] = {
+ 140, 204, 332, 588, 1100, 2116, 4156, 8244,
+};
+
+static inline int ina3221_wait_for_data(struct ina3221_data *ina)
+{
+ 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 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);
+
+ /* Polling the CVRF bit to make sure read data is ready */
+ return regmap_field_read_poll_timeout(ina->fields[F_CVRF],
+ cvrf, cvrf, wait, 10);
+}
+
static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
int *val)
{
@@ -150,6 +183,10 @@ static int ina3221_read_in(struct device *dev, u32 attr,
int channel, long *val)
if (!ina3221_is_enabled(ina, channel))
return -ENODATA;
+ ret = ina3221_wait_for_data(ina);
+ if (ret)
+ return ret;
+
ret = ina3221_read_value(ina, reg, );
if (ret)
return ret;
@@ -189,6 +226,11 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
case hwmon_curr_input:
if (!ina3221_is_enabled(ina, channel))
return -ENODATA;
+
+ ret = ina3221_wait_for_data(ina);
+ if (ret)
+ return ret;
+
/* fall through */
case hwmon_curr_crit:
case hwmon_curr_max:
--
2.17.1
[PATCH v3 1/5] hwmon: (core) Inherit power properties to hdev
The new hdev is a child device related to the original parent
hwmon driver and its device. However, it doesn't support the
power features, typically being defined in the parent driver.
So this patch inherits three necessary power properties from
the parent dev to hdev: power, pm_domain and driver pointers.
Note that the dev->driver pointer is the place that contains
a dev_pm_ops pointer defined in the parent device driver and
the pm runtime core also checks this pointer:
if (!cb && dev->driver && dev->driver->pm)
Signed-off-by: Nicolin Chen
---
Changelog
v2->v3:
* N/A
v1->v2:
* Added device pointers
drivers/hwmon/hwmon.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 975c95169884..14cfab64649f 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -625,7 +625,12 @@ __hwmon_device_register(struct device *dev, const char
*name, void *drvdata,
hwdev->name = name;
hdev->class = _class;
hdev->parent = dev;
- hdev->of_node = dev ? dev->of_node : NULL;
+ if (dev) {
+ hdev->driver = dev->driver;
+ hdev->power = dev->power;
+ hdev->pm_domain = dev->pm_domain;
+ hdev->of_node = dev->of_node;
+ }
hwdev->chip = chip;
dev_set_drvdata(hdev, drvdata);
dev_set_name(hdev, HWMON_ID_FORMAT, id);
--
2.17.1
[PATCH v3 5/5] hwmon: (ina3221) Add PM runtime support
If all three channels are disabled via in[123]_enable ABI,
the driver could suspend the chip for power saving purpose.
So this patch adds the PM runtime support in order to gain
more power control than system suspend and resume use case.
For PM runtime, there are a few related changes happening:
1) Added a new explicit hdev device pointer for all the PM
runtime callbacks. This is because hwmon core registers
a child device for each hwmon driver. So there might be
a mismatch between two device pointers in the driver if
mixing using them.
2) Added a check in ina3221_is_enabled() to make sure that
the chip is resumed.
3) Bypassed the unchanged status in ina3221_write_enable()
in order to keep the PM runtime refcount being matched.
4) Removed the reset routine in the probe() by calling the
resume() via pm_runtime_get_sync() instead, as they're
similar. It's also necessary to do so to match initial
PM refcount with the number of enabled channels.
5) Bypassed the system suspend/resume callbacks from the
i2c_client->dev, because both the i2c_client->dev and
the ina->hdev coexist and share the same pair of system
suspend/resume callback functions, which means there'd
be two suspend() calls during the system suspend while
the second one will fail.
Signed-off-by: Nicolin Chen
---
Changelog
v2->v3:
* Improved a dev_err message
* Added comments at pm_runtime_put_noidle() callbacks
* Added pm_runtime header file in an alphabetical order
v1->v2:
* Bypassed i2c_client->dev in suspend/resume()
* Added a missing '\n' in one dev_err()
drivers/hwmon/ina3221.c | 113
1 file changed, 91 insertions(+), 22 deletions(-)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 07dd6ef58d3e..3e7c6fac6e1b 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -20,6 +20,7 @@
#include
#include
#include
+#include
#include
#define INA3221_DRIVER_NAME"ina3221"
@@ -53,6 +54,7 @@
#define INA3221_CONFIG_CHs_EN_MASK GENMASK(14, 12)
#define INA3221_CONFIG_CHx_EN(x) BIT(14 - (x))
+#define INA3221_CONFIG_DEFAULT 0x7127
#define INA3221_RSHUNT_DEFAULT 1
enum ina3221_fields {
@@ -103,6 +105,7 @@ struct ina3221_input {
/**
* struct ina3221_data - device specific information
+ * @hdev: Device pointer of hwmon child device, used for pm runtime
* @regmap: Register map of the device
* @fields: Register fields of the device
* @inputs: Array of channel input source specific structures
@@ -110,6 +113,7 @@ struct ina3221_input {
* @reg_config: Register value of INA3221_CONFIG
*/
struct ina3221_data {
+ struct device *hdev;
struct regmap *regmap;
struct regmap_field *fields[F_MAX_FIELDS];
struct ina3221_input inputs[INA3221_NUM_CHANNELS];
@@ -119,7 +123,8 @@ struct ina3221_data {
static inline bool ina3221_is_enabled(struct ina3221_data *ina, int channel)
{
- return ina->reg_config & INA3221_CONFIG_CHx_EN(channel);
+ return pm_runtime_active(ina->hdev) &&
+ (ina->reg_config & INA3221_CONFIG_CHx_EN(channel));
}
/* Lookup table for Bus and Shunt conversion times in usec */
@@ -290,21 +295,48 @@ static int ina3221_write_enable(struct device *dev, int
channel, bool enable)
{
struct ina3221_data *ina = dev_get_drvdata(dev);
u16 config, mask = INA3221_CONFIG_CHx_EN(channel);
+ u16 config_old = ina->reg_config & mask;
int ret;
config = enable ? mask : 0;
+ /* Bypass if enable status is not being changed */
+ if (config_old == config)
+ return 0;
+
+ /* For enabling routine, increase refcount and resume() at first */
+ if (enable) {
+ ret = pm_runtime_get_sync(ina->hdev);
+ if (ret < 0) {
+ dev_err(dev, "Failed to get PM runtime\n");
+ return ret;
+ }
+ }
+
/* Enable or disable the channel */
ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, config);
if (ret)
- return ret;
+ goto fail;
/* Cache the latest config register value */
ret = regmap_read(ina->regmap, INA3221_CONFIG, >reg_config);
if (ret)
- return ret;
+ goto fail;
+
+ /* For disabling routine, decrease refcount or suspend() at last */
+ if (!enable)
+ pm_runtime_put_sync(ina->hdev);
return 0;
+
+fail:
+ if (enable) {
+ dev_err(dev, "Failed to enable channel %d: error %d\n",
+ channel, ret);
+ pm_runtime_put_sync(ina->hdev);
+ }
+
+ return ret;
}
static int ina3221_read(struct device *dev, enum hwmon_sensor_types type,
@@ -599,7 +631,6 @@ static int ina32
[PATCH v3 2/5] hwmon: (ina3221) Check channel status for alarms attribute read
There is nothing critically wrong to read these two attributes
without having a is_enabled() check at this point. But reading
the MASK_ENABLE register would clear the CVRF bit according to
the datasheet. So it'd be safer to fence for disabled channels
in order to add pm runtime feature.
Signed-off-by: Nicolin Chen
---
Changelog
v2->v3:
* N/A
v1->v2:
* Returned 0 for alert flags instead of -ENODATA
drivers/hwmon/ina3221.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index d61688f04594..26cdf3342d80 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -200,6 +200,12 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
return 0;
case hwmon_curr_crit_alarm:
case hwmon_curr_max_alarm:
+ /* No actual register read if channel is disabled */
+ if (!ina3221_is_enabled(ina, channel)) {
+ /* Return 0 for alert flags */
+ *val = 0;
+ return 0;
+ }
ret = regmap_field_read(ina->fields[reg], );
if (ret)
return ret;
--
2.17.1
[PATCH v3 3/5] hwmon: (ina3221) Serialize sysfs ABI accesses
This change adds a mutex to serialize accesses of sysfs attributes.
This is required when polling CVRF bit of the MASK/ENABLE register
because this bit is cleared on a read of this MASK/ENABLE register
or a write to CONFIG register, which means that this bit might be
accidentally cleared by reading other fields like alert flags.
So this patch adds a mutex lock to protect the write() and read()
callbacks. The read_string() callback won't need the lock since it
just returns the label without touching any hardware register.
Signed-off-by: Nicolin Chen
---
Changlog
v1->v3:
* N/A
drivers/hwmon/ina3221.c | 51 -
1 file changed, 45 insertions(+), 6 deletions(-)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 26cdf3342d80..10e8347a3c80 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
#include
#include
@@ -94,12 +95,14 @@ struct ina3221_input {
* @regmap: Register map of the device
* @fields: Register fields of the device
* @inputs: Array of channel input source specific structures
+ * @lock: mutex lock to serialize sysfs attribute accesses
* @reg_config: Register value of INA3221_CONFIG
*/
struct ina3221_data {
struct regmap *regmap;
struct regmap_field *fields[F_MAX_FIELDS];
struct ina3221_input inputs[INA3221_NUM_CHANNELS];
+ struct mutex lock;
u32 reg_config;
};
@@ -265,29 +268,53 @@ static int ina3221_write_enable(struct device *dev, int
channel, bool enable)
static int ina3221_read(struct device *dev, enum hwmon_sensor_types type,
u32 attr, int channel, long *val)
{
+ struct ina3221_data *ina = dev_get_drvdata(dev);
+ int ret;
+
+ mutex_lock(>lock);
+
switch (type) {
case hwmon_in:
/* 0-align channel ID */
- return ina3221_read_in(dev, attr, channel - 1, val);
+ ret = ina3221_read_in(dev, attr, channel - 1, val);
+ break;
case hwmon_curr:
- return ina3221_read_curr(dev, attr, channel, val);
+ ret = ina3221_read_curr(dev, attr, channel, val);
+ break;
default:
- return -EOPNOTSUPP;
+ ret = -EOPNOTSUPP;
+ break;
}
+
+ mutex_unlock(>lock);
+
+ return ret;
}
static int ina3221_write(struct device *dev, enum hwmon_sensor_types type,
u32 attr, int channel, long val)
{
+ struct ina3221_data *ina = dev_get_drvdata(dev);
+ int ret;
+
+ mutex_lock(>lock);
+
switch (type) {
case hwmon_in:
/* 0-align channel ID */
- return ina3221_write_enable(dev, channel - 1, val);
+ ret = ina3221_write_enable(dev, channel - 1, val);
+ break;
case hwmon_curr:
- return ina3221_write_curr(dev, attr, channel, val);
+ ret = ina3221_write_curr(dev, attr, channel, val);
+ break;
default:
- return -EOPNOTSUPP;
+ ret = -EOPNOTSUPP;
+ break;
}
+
+ mutex_unlock(>lock);
+
+ return ret;
}
static int ina3221_read_string(struct device *dev, enum hwmon_sensor_types
type,
@@ -582,6 +609,7 @@ static int ina3221_probe(struct i2c_client *client,
if (ret)
return ret;
+ mutex_init(>lock);
dev_set_drvdata(dev, ina);
hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, ina,
@@ -589,12 +617,22 @@ static int ina3221_probe(struct i2c_client *client,
ina3221_groups);
if (IS_ERR(hwmon_dev)) {
dev_err(dev, "Unable to register hwmon device\n");
+ mutex_destroy(>lock);
return PTR_ERR(hwmon_dev);
}
return 0;
}
+static int ina3221_remove(struct i2c_client *client)
+{
+ struct ina3221_data *ina = dev_get_drvdata(>dev);
+
+ mutex_destroy(>lock);
+
+ return 0;
+}
+
static int __maybe_unused ina3221_suspend(struct device *dev)
{
struct ina3221_data *ina = dev_get_drvdata(dev);
@@ -663,6 +701,7 @@ MODULE_DEVICE_TABLE(i2c, ina3221_ids);
static struct i2c_driver ina3221_i2c_driver = {
.probe = ina3221_probe,
+ .remove = ina3221_remove,
.driver = {
.name = INA3221_DRIVER_NAME,
.of_match_table = ina3221_of_match_table,
--
2.17.1
Re: [PATCH v2 5/5] hwmon: (ina3221) Add PM runtime support
On Wed, Oct 24, 2018 at 09:24:18AM +, li...@roeck-us.net wrote:
> > +fail:
> > + if (enable) {
> > + dev_err(dev, "Reverting channel%d enabling: %d\n",
> > + channel, ret);
>
> This message is confusing. Something like "Failed to enable channel %d:
> error %d" would be much better.
Will fix in v3.
> > +fail_pm:
> > + pm_runtime_disable(ina->hdev);
> > + pm_runtime_set_suspended(ina->hdev);
> > + for (i = 0; i < INA3221_NUM_CHANNELS; i++)
> > + pm_runtime_put_noidle(ina->hdev);
>
> The count here doesn't match the count above if some channels
> are disabled, or if the enable loop above aborted.
>
> > + /* Decrease the PM refcount */
> > + for (i = 0; i < INA3221_NUM_CHANNELS; i++)
> > + pm_runtime_put_noidle(ina->hdev);
> >
> As above, this doesn't take disabled channels into account. Maybe that
> doesn't matter; if so, there needs to be a comment indicating that
> negative use counts don't matter. If that is the case, make sure that
> this is acceptable use of the pm API (if it works but is not documented,
> the PM core may change and complain about it at a later time).
The API will stop decrease at 0. But I will see if I can explicitly
loop a matched number, or at lest will mention this in comments.
Thanks
Nicolin
Re: [PATCH v2 4/5] hwmon: (ina3221) Make sure data is ready before reading
On Wed, Oct 24, 2018 at 09:10:35AM +, li...@roeck-us.net wrote:
> > @@ -150,6 +183,12 @@ static int ina3221_read_in(struct device *dev, u32
> > attr, int channel, long *val)
> > if (!ina3221_is_enabled(ina, channel))
> > return -ENODATA;
> >
> > + ret = ina3221_wait_for_data(ina);
> > + if (ret) {
> > + dev_err(dev, "Timed out at waiting for CVRF bit\n");
> > + return ret;
> > + }
>
> Thanks for explaining why we can't just blindly wait.
> However, I am concerned about this log message: If something is wrong
> with the chip, this will spam the kernel log. Can you drop the message
> here and below ? After all, the error will be reported to userspace,
> and a kernel log message should not be necessary.
Will do that.
Thanks
Nicolin
>
> Thanks,
> Guenter
>
> > +
> > ret = ina3221_read_value(ina, reg, );
> > if (ret)
> > return ret;
> > @@ -189,6 +228,13 @@ static int ina3221_read_curr(struct device *dev,
> > u32 attr,
> > case hwmon_curr_input:
> > if (!ina3221_is_enabled(ina, channel))
> > return -ENODATA;
> > +
> > + ret = ina3221_wait_for_data(ina);
> > + if (ret) {
> > + dev_err(dev, "Timed out at waiting for CVRF bit\n");
> > + return ret;
> > + }
> > +
> > /* fall through */
> > case hwmon_curr_crit:
> > case hwmon_curr_max:
> > --
> > 2.17.1
>
>
>
[PATCH v2 2/5] hwmon: (ina3221) Check channel status for alarms attribute read
There is nothing critically wrong to read these two attributes
without having a is_enabled() check at this point. But reading
the MASK_ENABLE register would clear the CVRF bit according to
the datasheet. So it'd be safer to fence for disabled channels
in order to add pm runtime feature.
Signed-off-by: Nicolin Chen
---
Changelog
v1->v2:
* Returned 0 for alert flags instead of -ENODATA
drivers/hwmon/ina3221.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index d61688f04594..26cdf3342d80 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -200,6 +200,12 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
return 0;
case hwmon_curr_crit_alarm:
case hwmon_curr_max_alarm:
+ /* No actual register read if channel is disabled */
+ if (!ina3221_is_enabled(ina, channel)) {
+ /* Return 0 for alert flags */
+ *val = 0;
+ return 0;
+ }
ret = regmap_field_read(ina->fields[reg], );
if (ret)
return ret;
--
2.17.1
[PATCH v2 3/5] hwmon: (ina3221) Serialize sysfs ABI accesses
This change adds a mutex to serialize accesses of sysfs attributes.
This is required when polling CVRF bit of the MASK/ENABLE register
because this bit is cleared on a read of this MASK/ENABLE register
or a write to CONFIG register, which means that this bit might be
accidentally cleared by reading other fields like alert flags.
So this patch adds a mutex lock to protect the write() and read()
callbacks. The read_string() callback won't need the lock since it
just returns the label without touching any hardware register.
Signed-off-by: Nicolin Chen
---
drivers/hwmon/ina3221.c | 51 -
1 file changed, 45 insertions(+), 6 deletions(-)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 26cdf3342d80..10e8347a3c80 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
#include
#include
@@ -94,12 +95,14 @@ struct ina3221_input {
* @regmap: Register map of the device
* @fields: Register fields of the device
* @inputs: Array of channel input source specific structures
+ * @lock: mutex lock to serialize sysfs attribute accesses
* @reg_config: Register value of INA3221_CONFIG
*/
struct ina3221_data {
struct regmap *regmap;
struct regmap_field *fields[F_MAX_FIELDS];
struct ina3221_input inputs[INA3221_NUM_CHANNELS];
+ struct mutex lock;
u32 reg_config;
};
@@ -265,29 +268,53 @@ static int ina3221_write_enable(struct device *dev, int
channel, bool enable)
static int ina3221_read(struct device *dev, enum hwmon_sensor_types type,
u32 attr, int channel, long *val)
{
+ struct ina3221_data *ina = dev_get_drvdata(dev);
+ int ret;
+
+ mutex_lock(>lock);
+
switch (type) {
case hwmon_in:
/* 0-align channel ID */
- return ina3221_read_in(dev, attr, channel - 1, val);
+ ret = ina3221_read_in(dev, attr, channel - 1, val);
+ break;
case hwmon_curr:
- return ina3221_read_curr(dev, attr, channel, val);
+ ret = ina3221_read_curr(dev, attr, channel, val);
+ break;
default:
- return -EOPNOTSUPP;
+ ret = -EOPNOTSUPP;
+ break;
}
+
+ mutex_unlock(>lock);
+
+ return ret;
}
static int ina3221_write(struct device *dev, enum hwmon_sensor_types type,
u32 attr, int channel, long val)
{
+ struct ina3221_data *ina = dev_get_drvdata(dev);
+ int ret;
+
+ mutex_lock(>lock);
+
switch (type) {
case hwmon_in:
/* 0-align channel ID */
- return ina3221_write_enable(dev, channel - 1, val);
+ ret = ina3221_write_enable(dev, channel - 1, val);
+ break;
case hwmon_curr:
- return ina3221_write_curr(dev, attr, channel, val);
+ ret = ina3221_write_curr(dev, attr, channel, val);
+ break;
default:
- return -EOPNOTSUPP;
+ ret = -EOPNOTSUPP;
+ break;
}
+
+ mutex_unlock(>lock);
+
+ return ret;
}
static int ina3221_read_string(struct device *dev, enum hwmon_sensor_types
type,
@@ -582,6 +609,7 @@ static int ina3221_probe(struct i2c_client *client,
if (ret)
return ret;
+ mutex_init(>lock);
dev_set_drvdata(dev, ina);
hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, ina,
@@ -589,12 +617,22 @@ static int ina3221_probe(struct i2c_client *client,
ina3221_groups);
if (IS_ERR(hwmon_dev)) {
dev_err(dev, "Unable to register hwmon device\n");
+ mutex_destroy(>lock);
return PTR_ERR(hwmon_dev);
}
return 0;
}
+static int ina3221_remove(struct i2c_client *client)
+{
+ struct ina3221_data *ina = dev_get_drvdata(>dev);
+
+ mutex_destroy(>lock);
+
+ return 0;
+}
+
static int __maybe_unused ina3221_suspend(struct device *dev)
{
struct ina3221_data *ina = dev_get_drvdata(dev);
@@ -663,6 +701,7 @@ MODULE_DEVICE_TABLE(i2c, ina3221_ids);
static struct i2c_driver ina3221_i2c_driver = {
.probe = ina3221_probe,
+ .remove = ina3221_remove,
.driver = {
.name = INA3221_DRIVER_NAME,
.of_match_table = ina3221_of_match_table,
--
2.17.1
[PATCH v2 0/5] hwmon: (ina3221) Implement PM runtime to save power
This series patches implement PM runtime feature in the ina3221 hwmon driver (PATCH-5). However, PATCH-[1:4] are required to make sure that the PM runtime feature would be functional and safe. Changelog v1->v2: * Added device pointer check (PATCH-1) * Returned 0 for alert flags (PATCH-2) * Moved CVRF polling to data read routine (PATCH-4) * Bypassed i2c_client->dev in suspend/resume() (PATCH-5) Nicolin Chen (5): hwmon: (core) Inherit power properties to hdev hwmon: (ina3221) Check channel status for alarms attribute read hwmon: (ina3221) Serialize sysfs ABI accesses hwmon: (ina3221) Make sure data is ready before reading hwmon: (ina3221) Add PM runtime support drivers/hwmon/hwmon.c | 7 +- drivers/hwmon/ina3221.c | 213 +++- 2 files changed, 192 insertions(+), 28 deletions(-) -- 2.17.1
[PATCH v2 1/5] hwmon: (core) Inherit power properties to hdev
The new hdev is a child device related to the original parent
hwmon driver and its device. However, it doesn't support the
power features, typically being defined in the parent driver.
So this patch inherits three necessary power properties from
the parent dev to hdev: power, pm_domain and driver pointers.
Note that the dev->driver pointer is the place that contains
a dev_pm_ops pointer defined in the parent device driver and
the pm runtime core also checks this pointer:
if (!cb && dev->driver && dev->driver->pm)
Signed-off-by: Nicolin Chen
---
Changelog
v1->v2:
* Added device pointer check
drivers/hwmon/hwmon.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 975c95169884..14cfab64649f 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -625,7 +625,12 @@ __hwmon_device_register(struct device *dev, const char
*name, void *drvdata,
hwdev->name = name;
hdev->class = _class;
hdev->parent = dev;
- hdev->of_node = dev ? dev->of_node : NULL;
+ if (dev) {
+ hdev->driver = dev->driver;
+ hdev->power = dev->power;
+ hdev->pm_domain = dev->pm_domain;
+ hdev->of_node = dev->of_node;
+ }
hwdev->chip = chip;
dev_set_drvdata(hdev, drvdata);
dev_set_name(hdev, HWMON_ID_FORMAT, id);
--
2.17.1
[PATCH v2 4/5] hwmon: (ina3221) Make sure data is ready before reading
The data might need some time to get ready after channel enabling,
although the data register is always readable. The CVRF bit is to
indicate that data conversion is finished, so polling the CVRF bit
before data reading could ensure the result being valid.
An alternative way could be to wait for expected time between the
channel enabling and the data reading. And this could avoid extra
I2C communications. However, INA3221 seemly takes longer time than
what's stated in the datasheet. Test results show that sometimes
it couldn't finish data conversion in time.
So this patch plays safe by adding a CVRF polling to make sure the
data register is updated with the new data.
Signed-off-by: Nicolin Chen
---
* Moved CVRF polling to data read routine
* Added calculation of wait time based on conversion time setting
drivers/hwmon/ina3221.c | 46 +
1 file changed, 46 insertions(+)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 10e8347a3c80..9bbac826e50b 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -44,6 +44,13 @@
#define INA3221_CONFIG_MODE_SHUNT BIT(0)
#define INA3221_CONFIG_MODE_BUSBIT(1)
#define INA3221_CONFIG_MODE_CONTINUOUS BIT(2)
+#define INA3221_CONFIG_VSH_CT_SHIFT3
+#define INA3221_CONFIG_VSH_CT_MASK GENMASK(5, 3)
+#define INA3221_CONFIG_VSH_CT(x) (((x) & GENMASK(5, 3)) >> 3)
+#define INA3221_CONFIG_VBUS_CT_SHIFT 6
+#define INA3221_CONFIG_VBUS_CT_MASKGENMASK(8, 6)
+#define INA3221_CONFIG_VBUS_CT(x) (((x) & GENMASK(8, 6)) >> 6)
+#define INA3221_CONFIG_CHs_EN_MASK GENMASK(14, 12)
#define INA3221_CONFIG_CHx_EN(x) BIT(14 - (x))
#define INA3221_RSHUNT_DEFAULT 1
@@ -52,6 +59,9 @@ enum ina3221_fields {
/* Configuration */
F_RST,
+ /* Status Flags */
+ F_CVRF,
+
/* Alert Flags */
F_WF3, F_WF2, F_WF1,
F_CF3, F_CF2, F_CF1,
@@ -63,6 +73,7 @@ enum ina3221_fields {
static const struct reg_field ina3221_reg_fields[] = {
[F_RST] = REG_FIELD(INA3221_CONFIG, 15, 15),
+ [F_CVRF] = REG_FIELD(INA3221_MASK_ENABLE, 0, 0),
[F_WF3] = REG_FIELD(INA3221_MASK_ENABLE, 3, 3),
[F_WF2] = REG_FIELD(INA3221_MASK_ENABLE, 4, 4),
[F_WF1] = REG_FIELD(INA3221_MASK_ENABLE, 5, 5),
@@ -111,6 +122,28 @@ static inline bool ina3221_is_enabled(struct ina3221_data
*ina, int channel)
return ina->reg_config & INA3221_CONFIG_CHx_EN(channel);
}
+/* Lookup table for Bus and Shunt conversion times in usec */
+static const u16 ina3221_conv_time[] = {
+ 140, 204, 332, 588, 1100, 2116, 4156, 8244,
+};
+
+static inline int ina3221_wait_for_data(struct ina3221_data *ina)
+{
+ 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 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);
+
+ /* Polling the CVRF bit to make sure read data is ready */
+ return regmap_field_read_poll_timeout(ina->fields[F_CVRF],
+ cvrf, cvrf, wait, 10);
+}
+
static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
int *val)
{
@@ -150,6 +183,12 @@ static int ina3221_read_in(struct device *dev, u32 attr,
int channel, long *val)
if (!ina3221_is_enabled(ina, channel))
return -ENODATA;
+ ret = ina3221_wait_for_data(ina);
+ if (ret) {
+ dev_err(dev, "Timed out at waiting for CVRF bit\n");
+ return ret;
+ }
+
ret = ina3221_read_value(ina, reg, );
if (ret)
return ret;
@@ -189,6 +228,13 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
case hwmon_curr_input:
if (!ina3221_is_enabled(ina, channel))
return -ENODATA;
+
+ ret = ina3221_wait_for_data(ina);
+ if (ret) {
+ dev_err(dev, "Timed out at waiting for CVRF bit\n");
+ return ret;
+ }
+
/* fall through */
case hwmon_curr_crit:
case hwmon_curr_max:
--
2.17.1
[PATCH v2 5/5] hwmon: (ina3221) Add PM runtime support
If all three channels are disabled via in[123]_enable ABI,
the driver could suspend the chip for power saving purpose.
So this patch addsd the PM runtime support in order to gain
more power control than system suspend and resume use case.
For PM runtime, there are a few related changes happening:
1) Added a new explicit hdev device pointer for all the pm
runtime callbacks. This is because hwmon core registers
a child device for each hwmon driver. So there might be
a mismatch between two device pointers in the driver if
mixing using them.
2) Added a check in ina3221_is_enabled() to make sure that
the chip is resumed.
3) Bypassed the unchanged status in ina3221_write_enable()
in order to keep the pm runtime refcount being matched.
4) Removed the reset routine in the probe() by calling the
resume() via pm_runtime_get_sync(), as they're similar.
It's also necessary to do so to match initial refcount
with the number of enabled channels.
5) Bypassed the system suspend/resume callbacks from the
i2c_client->dev, because both the i2c_client->dev and
the ina->hdev coexist and share the same pair of system
suspend/resume callback functions, which means there'd
be two suspend() calls during the system suspend while
the second one will fail.
Signed-off-by: Nicolin Chen
---
Changelog
v1->v2:
* Bypassed i2c_client->dev in suspend/resume()
* Added a missing '\n' in one dev_err()
drivers/hwmon/ina3221.c | 112
1 file changed, 90 insertions(+), 22 deletions(-)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 9bbac826e50b..2cdc37ab0cf3 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -21,6 +21,7 @@
#include
#include
#include
+#include
#define INA3221_DRIVER_NAME"ina3221"
@@ -53,6 +54,7 @@
#define INA3221_CONFIG_CHs_EN_MASK GENMASK(14, 12)
#define INA3221_CONFIG_CHx_EN(x) BIT(14 - (x))
+#define INA3221_CONFIG_DEFAULT 0x7127
#define INA3221_RSHUNT_DEFAULT 1
enum ina3221_fields {
@@ -103,6 +105,7 @@ struct ina3221_input {
/**
* struct ina3221_data - device specific information
+ * @hdev: Device pointer of hwmon child device, used for pm runtime
* @regmap: Register map of the device
* @fields: Register fields of the device
* @inputs: Array of channel input source specific structures
@@ -110,6 +113,7 @@ struct ina3221_input {
* @reg_config: Register value of INA3221_CONFIG
*/
struct ina3221_data {
+ struct device *hdev;
struct regmap *regmap;
struct regmap_field *fields[F_MAX_FIELDS];
struct ina3221_input inputs[INA3221_NUM_CHANNELS];
@@ -119,7 +123,8 @@ struct ina3221_data {
static inline bool ina3221_is_enabled(struct ina3221_data *ina, int channel)
{
- return ina->reg_config & INA3221_CONFIG_CHx_EN(channel);
+ return pm_runtime_active(ina->hdev) &&
+ (ina->reg_config & INA3221_CONFIG_CHx_EN(channel));
}
/* Lookup table for Bus and Shunt conversion times in usec */
@@ -294,21 +299,48 @@ static int ina3221_write_enable(struct device *dev, int
channel, bool enable)
{
struct ina3221_data *ina = dev_get_drvdata(dev);
u16 config, mask = INA3221_CONFIG_CHx_EN(channel);
+ u16 config_old = ina->reg_config & mask;
int ret;
config = enable ? mask : 0;
+ /* Bypass if enable status is not being changed */
+ if (config_old == config)
+ return 0;
+
+ /* For enabling routine, increase refcount and resume() at first */
+ if (enable) {
+ ret = pm_runtime_get_sync(ina->hdev);
+ if (ret < 0) {
+ dev_err(dev, "Failed to get PM runtime\n");
+ return ret;
+ }
+ }
+
/* Enable or disable the channel */
ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, config);
if (ret)
- return ret;
+ goto fail;
/* Cache the latest config register value */
ret = regmap_read(ina->regmap, INA3221_CONFIG, >reg_config);
if (ret)
- return ret;
+ goto fail;
+
+ /* For disabling routine, decrease refcount or suspend() at last */
+ if (!enable)
+ pm_runtime_put_sync(ina->hdev);
return 0;
+
+fail:
+ if (enable) {
+ dev_err(dev, "Reverting channel%d enabling: %d\n",
+ channel, ret);
+ pm_runtime_put_sync(ina->hdev);
+ }
+
+ return ret;
}
static int ina3221_read(struct device *dev, enum hwmon_sensor_types type,
@@ -603,7 +635,6 @@ static int ina3221_probe(struct i2c_client *client,
{
struct device *dev = >dev;
struct ina3221_data *ina;
- struct device *hwmon_dev;
int i, ret;
ina
Re: [PATCH 4/5] hwmon: (ina3221) Make sure data is ready after channel enabling
On Wed, Oct 17, 2018 at 02:17:14PM -0700, Guenter Roeck wrote: > On Wed, Oct 17, 2018 at 01:53:48PM -0700, Nicolin Chen wrote: > > Hello Guenter, > > > > On Wed, Oct 17, 2018 at 09:55:43AM -0700, Guenter Roeck wrote: > > > > @@ -676,6 +701,13 @@ static int __maybe_unused ina3221_resume(struct > > > > device *dev) > > > > if (ret) > > > > return ret; > > > > > > > > + /* Make sure data conversion is finished */ > > > > + ret = ina3221_wait_for_data_if_active(ina); > > > > > > This is quite expensive and would delay resume (and enable, for that > > > matter). > > > A less expensive solution would be to save the enable time here and in > > > ina3221_write_enable(). ina3221_wait_for_data_if_active() can then be > > > called > > > from read functions and would wait if not enough time has expired. > > > > > > if (time_before(...)) > > > usleep_range(missing wait time, missing_wait_time * 2); > > > > > > or something like that. This could be done per channel or, to keep > > > things simple, just using a single time for all channels. > > > > I was thinking something that'd fit one-shot mode too so decided > > to add a polling. But you are right. It does make sense to skip > > polling until an actual read happens, though it also feels a bit > > reasonable to me that putting a polling to the enabling routine. > > > > The wait time would be sightly more complicated than the polling > > actually, as it needs to involve total conversion time which may > > vary depending on the number of enabled channels. I will see what > > would be safer and easier and apply that in v2. > > > It isn't that complex; we have done it in other drivers. It is less > costly and has less overhead than extra i2c read operation(s). OK. Will follow them in v2. Thanks Nicolin
Re: [PATCH 4/5] hwmon: (ina3221) Make sure data is ready after channel enabling
Hello Guenter, On Wed, Oct 17, 2018 at 09:55:43AM -0700, Guenter Roeck wrote: > > @@ -676,6 +701,13 @@ static int __maybe_unused ina3221_resume(struct device > > *dev) > > if (ret) > > return ret; > > > > + /* Make sure data conversion is finished */ > > + ret = ina3221_wait_for_data_if_active(ina); > > This is quite expensive and would delay resume (and enable, for that matter). > A less expensive solution would be to save the enable time here and in > ina3221_write_enable(). ina3221_wait_for_data_if_active() can then be called > from read functions and would wait if not enough time has expired. > > if (time_before(...)) > usleep_range(missing wait time, missing_wait_time * 2); > > or something like that. This could be done per channel or, to keep > things simple, just using a single time for all channels. I was thinking something that'd fit one-shot mode too so decided to add a polling. But you are right. It does make sense to skip polling until an actual read happens, though it also feels a bit reasonable to me that putting a polling to the enabling routine. The wait time would be sightly more complicated than the polling actually, as it needs to involve total conversion time which may vary depending on the number of enabled channels. I will see what would be safer and easier and apply that in v2. Thanks Nicolin
Re: [PATCH 1/5] hwmon: (core) Inherit power properties to hdev
On Wed, Oct 17, 2018 at 12:44:30PM -0700, Guenter Roeck wrote: > > + hdev->driver = dev->driver; > > + hdev->power = dev->power; > > + hdev->pm_domain = dev->pm_domain; > > dev can, unfortunately, be NULL > > > hdev->of_node = dev ? dev->of_node : NULL; > > ... as you can see here. Oops..will fix it. Thanks! > > Guenter > > > hwdev->chip = chip; > > dev_set_drvdata(hdev, drvdata); > > -- > > 2.17.1 > >
[PATCH 2/5] hwmon: (ina3221) Return -ENODATA for two alarms attributes
There is nothing critically wrong to read these two attributes without having a is_enabled() check at this point. But reading the MASK_ENABLE register would clear the CVRF bit according to the datasheet. So it'd be safer to fence for disabled channels in order to add pm runtime feature. Signed-off-by: Nicolin Chen --- drivers/hwmon/ina3221.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c index d61688f04594..3e98b59108ee 100644 --- a/drivers/hwmon/ina3221.c +++ b/drivers/hwmon/ina3221.c @@ -200,6 +200,8 @@ static int ina3221_read_curr(struct device *dev, u32 attr, return 0; case hwmon_curr_crit_alarm: case hwmon_curr_max_alarm: + if (!ina3221_is_enabled(ina, channel)) + return -ENODATA; ret = regmap_field_read(ina->fields[reg], ); if (ret) return ret; -- 2.17.1
[PATCH 0/5] hwmon: (ina3221) Implement PM runtime to save power
This series patches implement PM runtime feature in the ina3221 hwmon driver (PATCH-5). However, PATCH-[1:4] are required to make sure that the PM runtime feature would be functional and safe. Nicolin Chen (5): hwmon: (core) Inherit power properties to hdev hwmon: (ina3221) Return -ENODATA for two alarms attributes hwmon: (ina3221) Serialize sysfs ABI accesses hwmon: (ina3221) Make sure data is ready after channel enabling hwmon: (ina3221) Add PM runtime support drivers/hwmon/hwmon.c | 3 + drivers/hwmon/ina3221.c | 185 ++-- 2 files changed, 162 insertions(+), 26 deletions(-) -- 2.17.1
[PATCH 1/5] hwmon: (core) Inherit power properties to hdev
The new hdev is a child device related to the original parent hwmon driver and its device. However, it doesn't support the power features, typically being defined in the parent driver. So this patch inherits three necessary power properties from the parent dev to hdev: power, pm_domain and driver pointers. Note that the dev->driver pointer is the place that contains a dev_pm_ops pointer defined in the parent device driver and the pm runtime core also checks this pointer: if (!cb && dev->driver && dev->driver->pm) Signed-off-by: Nicolin Chen --- drivers/hwmon/hwmon.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c index 975c95169884..7c064e1218ba 100644 --- a/drivers/hwmon/hwmon.c +++ b/drivers/hwmon/hwmon.c @@ -625,6 +625,9 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata, hwdev->name = name; hdev->class = _class; hdev->parent = dev; + hdev->driver = dev->driver; + hdev->power = dev->power; + hdev->pm_domain = dev->pm_domain; hdev->of_node = dev ? dev->of_node : NULL; hwdev->chip = chip; dev_set_drvdata(hdev, drvdata); -- 2.17.1
[PATCH 4/5] hwmon: (ina3221) Make sure data is ready after channel enabling
The data might need some time to get ready after channel enabling,
although the data register is readable without CVRF being set. So
this patch adds a CVRF polling to make sure that data register is
updated with the new data.
Signed-off-by: Nicolin Chen
---
drivers/hwmon/ina3221.c | 32
1 file changed, 32 insertions(+)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 5fc375bf6635..160ddc404d73 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -45,6 +45,7 @@
#define INA3221_CONFIG_MODE_BUSBIT(1)
#define INA3221_CONFIG_MODE_CONTINUOUS BIT(2)
#define INA3221_CONFIG_CHx_EN(x) BIT(14 - (x))
+#define INA3221_CONFIG_CHs_EN_MASK GENMASK(14, 12)
#define INA3221_RSHUNT_DEFAULT 1
@@ -52,6 +53,9 @@ enum ina3221_fields {
/* Configuration */
F_RST,
+ /* Status Flags */
+ F_CVRF,
+
/* Alert Flags */
F_WF3, F_WF2, F_WF1,
F_CF3, F_CF2, F_CF1,
@@ -63,6 +67,7 @@ enum ina3221_fields {
static const struct reg_field ina3221_reg_fields[] = {
[F_RST] = REG_FIELD(INA3221_CONFIG, 15, 15),
+ [F_CVRF] = REG_FIELD(INA3221_MASK_ENABLE, 0, 0),
[F_WF3] = REG_FIELD(INA3221_MASK_ENABLE, 3, 3),
[F_WF2] = REG_FIELD(INA3221_MASK_ENABLE, 4, 4),
[F_WF1] = REG_FIELD(INA3221_MASK_ENABLE, 5, 5),
@@ -111,6 +116,19 @@ static inline bool ina3221_is_enabled(struct ina3221_data
*ina, int channel)
return ina->reg_config & INA3221_CONFIG_CHx_EN(channel);
}
+static inline int ina3221_wait_for_data_if_active(struct ina3221_data *ina)
+{
+ u32 cvrf;
+
+ /* No need to wait if all channels are disabled */
+ if ((ina->reg_config & INA3221_CONFIG_CHs_EN_MASK) == 0)
+ return 0;
+
+ /* Polling the CVRF bit to make sure read data is ready */
+ return regmap_field_read_poll_timeout(ina->fields[F_CVRF],
+ cvrf, cvrf, 100, 10);
+}
+
static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
int *val)
{
@@ -258,6 +276,13 @@ static int ina3221_write_enable(struct device *dev, int
channel, bool enable)
if (ret)
return ret;
+ /* Make sure data conversion is finished */
+ ret = ina3221_wait_for_data_if_active(ina);
+ if (ret) {
+ dev_err(dev, "Timed out at waiting for CVRF bit\n");
+ return ret;
+ }
+
return 0;
}
@@ -676,6 +701,13 @@ static int __maybe_unused ina3221_resume(struct device
*dev)
if (ret)
return ret;
+ /* Make sure data conversion is finished */
+ ret = ina3221_wait_for_data_if_active(ina);
+ if (ret) {
+ dev_err(dev, "Timed out at waiting for CVRF bit\n");
+ return ret;
+ }
+
return 0;
}
--
2.17.1
[PATCH 3/5] hwmon: (ina3221) Serialize sysfs ABI accesses
This change adds a mutex to serialize accesses of sysfs attributes.
This is required when polling CVRF bit of the MASK/ENABLE register
because this bit is cleared on a read of this MASK/ENABLE register
or a write to CONFIG register, which means that this bit might be
accidentally cleared by reading other fields like alert flags.
So this patch adds a mutex lock to protect the write() and read()
callbacks. The read_string() callback won't need the lock since it
just returns the label without touching any hardware register.
Signed-off-by: Nicolin Chen
---
drivers/hwmon/ina3221.c | 51 -
1 file changed, 45 insertions(+), 6 deletions(-)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 3e98b59108ee..5fc375bf6635 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
#include
#include
@@ -94,12 +95,14 @@ struct ina3221_input {
* @regmap: Register map of the device
* @fields: Register fields of the device
* @inputs: Array of channel input source specific structures
+ * @lock: mutex lock to serialize sysfs attribute accesses
* @reg_config: Register value of INA3221_CONFIG
*/
struct ina3221_data {
struct regmap *regmap;
struct regmap_field *fields[F_MAX_FIELDS];
struct ina3221_input inputs[INA3221_NUM_CHANNELS];
+ struct mutex lock;
u32 reg_config;
};
@@ -261,29 +264,53 @@ static int ina3221_write_enable(struct device *dev, int
channel, bool enable)
static int ina3221_read(struct device *dev, enum hwmon_sensor_types type,
u32 attr, int channel, long *val)
{
+ struct ina3221_data *ina = dev_get_drvdata(dev);
+ int ret;
+
+ mutex_lock(>lock);
+
switch (type) {
case hwmon_in:
/* 0-align channel ID */
- return ina3221_read_in(dev, attr, channel - 1, val);
+ ret = ina3221_read_in(dev, attr, channel - 1, val);
+ break;
case hwmon_curr:
- return ina3221_read_curr(dev, attr, channel, val);
+ ret = ina3221_read_curr(dev, attr, channel, val);
+ break;
default:
- return -EOPNOTSUPP;
+ ret = -EOPNOTSUPP;
+ break;
}
+
+ mutex_unlock(>lock);
+
+ return ret;
}
static int ina3221_write(struct device *dev, enum hwmon_sensor_types type,
u32 attr, int channel, long val)
{
+ struct ina3221_data *ina = dev_get_drvdata(dev);
+ int ret;
+
+ mutex_lock(>lock);
+
switch (type) {
case hwmon_in:
/* 0-align channel ID */
- return ina3221_write_enable(dev, channel - 1, val);
+ ret = ina3221_write_enable(dev, channel - 1, val);
+ break;
case hwmon_curr:
- return ina3221_write_curr(dev, attr, channel, val);
+ ret = ina3221_write_curr(dev, attr, channel, val);
+ break;
default:
- return -EOPNOTSUPP;
+ ret = -EOPNOTSUPP;
+ break;
}
+
+ mutex_unlock(>lock);
+
+ return ret;
}
static int ina3221_read_string(struct device *dev, enum hwmon_sensor_types
type,
@@ -578,6 +605,7 @@ static int ina3221_probe(struct i2c_client *client,
if (ret)
return ret;
+ mutex_init(>lock);
dev_set_drvdata(dev, ina);
hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, ina,
@@ -585,12 +613,22 @@ static int ina3221_probe(struct i2c_client *client,
ina3221_groups);
if (IS_ERR(hwmon_dev)) {
dev_err(dev, "Unable to register hwmon device\n");
+ mutex_destroy(>lock);
return PTR_ERR(hwmon_dev);
}
return 0;
}
+static int ina3221_remove(struct i2c_client *client)
+{
+ struct ina3221_data *ina = dev_get_drvdata(>dev);
+
+ mutex_destroy(>lock);
+
+ return 0;
+}
+
static int __maybe_unused ina3221_suspend(struct device *dev)
{
struct ina3221_data *ina = dev_get_drvdata(dev);
@@ -659,6 +697,7 @@ MODULE_DEVICE_TABLE(i2c, ina3221_ids);
static struct i2c_driver ina3221_i2c_driver = {
.probe = ina3221_probe,
+ .remove = ina3221_remove,
.driver = {
.name = INA3221_DRIVER_NAME,
.of_match_table = ina3221_of_match_table,
--
2.17.1
[PATCH 5/5] hwmon: (ina3221) Add PM runtime support
If all three channels are disabled via in[123]_enable ABI,
the driver could suspend the chip for power saving purpose.
So this patch addsd the PM runtime support in order to gain
more power control than system suspend and resume use case.
For PM runtime, there are a few related changes happening:
1) Added a new explicit hdev device pointer for all the pm
runtime callbacks. This is because hwmon core registers
a child device for each hwmon driver. So there might be
a mismatch between two device pointers in the driver if
mixing using them.
2) Added a check in ina3221_is_enabled() to make sure that
the chip is resumed.
3) Bypassed the unchanged status in ina3221_write_enable()
in order to keep the pm runtime refcount being matched.
4) Removed the reset routine in the probe() by calling the
resume() via pm_runtime_get_sync(), as they're similar.
It's also necessary to do so to match initial refcount
with the number of enabled channels.
Signed-off-by: Nicolin Chen
---
drivers/hwmon/ina3221.c | 104 +++-
1 file changed, 82 insertions(+), 22 deletions(-)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 160ddc404d73..5ad3ab22b07a 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -21,6 +21,7 @@
#include
#include
#include
+#include
#define INA3221_DRIVER_NAME"ina3221"
@@ -47,6 +48,7 @@
#define INA3221_CONFIG_CHx_EN(x) BIT(14 - (x))
#define INA3221_CONFIG_CHs_EN_MASK GENMASK(14, 12)
+#define INA3221_CONFIG_DEFAULT 0x7127
#define INA3221_RSHUNT_DEFAULT 1
enum ina3221_fields {
@@ -97,6 +99,7 @@ struct ina3221_input {
/**
* struct ina3221_data - device specific information
+ * @hdev: Device pointer of hwmon child device, used for pm runtime
* @regmap: Register map of the device
* @fields: Register fields of the device
* @inputs: Array of channel input source specific structures
@@ -104,6 +107,7 @@ struct ina3221_input {
* @reg_config: Register value of INA3221_CONFIG
*/
struct ina3221_data {
+ struct device *hdev;
struct regmap *regmap;
struct regmap_field *fields[F_MAX_FIELDS];
struct ina3221_input inputs[INA3221_NUM_CHANNELS];
@@ -113,7 +117,8 @@ struct ina3221_data {
static inline bool ina3221_is_enabled(struct ina3221_data *ina, int channel)
{
- return ina->reg_config & INA3221_CONFIG_CHx_EN(channel);
+ return pm_runtime_active(ina->hdev) &&
+ (ina->reg_config & INA3221_CONFIG_CHx_EN(channel));
}
static inline int ina3221_wait_for_data_if_active(struct ina3221_data *ina)
@@ -262,19 +267,33 @@ static int ina3221_write_enable(struct device *dev, int
channel, bool enable)
{
struct ina3221_data *ina = dev_get_drvdata(dev);
u16 config, mask = INA3221_CONFIG_CHx_EN(channel);
+ u16 config_old = ina->reg_config & mask;
int ret;
config = enable ? mask : 0;
+ /* Bypass if enable status is not being changed */
+ if (config_old == config)
+ return 0;
+
+ /* For enabling routine, increase refcount and resume() at first */
+ if (enable) {
+ ret = pm_runtime_get_sync(ina->hdev);
+ if (ret < 0) {
+ dev_err(dev, "Failed to get PM runtime");
+ return ret;
+ }
+ }
+
/* Enable or disable the channel */
ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, config);
if (ret)
- return ret;
+ goto fail;
/* Cache the latest config register value */
ret = regmap_read(ina->regmap, INA3221_CONFIG, >reg_config);
if (ret)
- return ret;
+ goto fail;
/* Make sure data conversion is finished */
ret = ina3221_wait_for_data_if_active(ina);
@@ -283,7 +302,20 @@ static int ina3221_write_enable(struct device *dev, int
channel, bool enable)
return ret;
}
+ /* For disabling routine, decrease refcount or suspend() at last */
+ if (!enable)
+ pm_runtime_put_sync(ina->hdev);
+
return 0;
+
+fail:
+ if (enable) {
+ dev_err(dev, "Reverting channel%d enabling: %d\n",
+ channel, ret);
+ pm_runtime_put_sync(ina->hdev);
+ }
+
+ return ret;
}
static int ina3221_read(struct device *dev, enum hwmon_sensor_types type,
@@ -578,7 +610,6 @@ static int ina3221_probe(struct i2c_client *client,
{
struct device *dev = >dev;
struct ina3221_data *ina;
- struct device *hwmon_dev;
int i, ret;
ina = devm_kzalloc(dev, sizeof(*ina), GFP_KERNEL);
@@ -610,44 +641,71 @@ static int ina3221_probe(struct i2c_client *client,
return ret;
}
- ret = regmap
Re: [PATCH 2/2] hwmon: (ina3221) Add operating mode support
On Thu, Oct 11, 2018 at 12:31:52PM -0700, Guenter Roeck wrote: > > One more question here, and this might sound a bit abuse of using > > the existing hwmon ABI: would it sound plausible to you that the > > driver powers down the chip when all three channels get disabled > > via in[123]_enable nodes? :) > > > > I would not call that an abuse, no. Hmm..do you mean that you aren't in favor of powering down the chip after all channels get disabled? I was thinking about having pm_runtime_get_sync()/put() for channel enabling/disabling routine of in[123]_enable. Thanks Nicolin
Re: [PATCH 2/2] hwmon: (ina3221) Add operating mode support
Hi Guenter, On Wed, Oct 10, 2018 at 04:43:00PM -0700, Guenter Roeck wrote: > > > The effort to do all this using CPU cycles would in most if not all cases > > > outweigh any perceived power savings. As such, I just don't see the > > > practical use case. > > > > It really depends on the use case and how often the one-shot gets > > triggered. For battery-powered devices, running in the continuous > > mode does consume considerable power based on the measurement from > > our power folks. If a system is running in a power sensitive mode, > > while it still needs to occasionally check the inputs, it could be > > a use case for one-shot mode, though it's purely a user decision. > > > That would actually be a use case for runtime power management. > The power used by a modern sensor chip is miniscule compared > to the power consumed by other components in the system, > which may explain why no one bothered looking into runtime > power management for sensors. > > This is also an argument against any device or subsystem specific solution: > Users will want to be able to control power consumption for all devices > in the system, not just for sensors. A device specific power control > mechanism would, from user space perspective, be a nightmare. That makes sense to me. Actually we wanted a solution more on the driver side so that user space doesn't need to be involved. But our downstream solution (switching different modes when certain number of CPUs get hot plugged in/out) wasn't accepted years ago by other subsystem maintainers, being commented that it's a user decision, IIRC. So I thought that having a sysfs node might be a generic way for "user decision". But I guess I should have tried to seek for a solution in the kernel like runtime PM as you said. > > > power-down mode effectively reinvents runtime power management (runtime > > > suspend/resume support) and is thus simply unacceptable. > > > > Similar to one-shot, if a system is in a low power mode where it > > doesn't want to check the inputs anymore, I feel the user space > > could at least make the decision to turn on/off the chips, I am > > not quite sure if the generic runtime PM system already has this > > kind of support though. > > > Please look up "runtime power management". It provides the basic > mechanism to turn off / disable a device if it is not used. The point > here is that the basic mechanism is there, even though it may not > be perfect. If it is not perfect, it needs to be improved. > Implementing a per-subsystem alternate method would be the wrong > approach. I have implemented runtime PM suspend/resume in other drivers but what I know is that it relies on someone to call get_sync and put functions accordingly and it doesn't seem to have built-in system level low-power or power-sensitive mode for those use cases that I mentioned previously. But I agree with your point and will take a closer look. One more question here, and this might sound a bit abuse of using the existing hwmon ABI: would it sound plausible to you that the driver powers down the chip when all three channels get disabled via in[123]_enable nodes? :) Thanks Nicolin
Re: [PATCH 2/2] hwmon: (ina3221) Add operating mode support
Hello Guenter,
On Wed, Oct 10, 2018 at 06:22:39AM -0700, Guenter Roeck wrote:
> > The hwmon core now has a new optional mode interface. So this patch
> > just implements this mode support so that user space can check and
> > configure via sysfs node its operating modes: power-down, one-shot,
> > and continuous modes.
> One-shot mode on its own does not make sense or add value: It would require
> explicit driver support to trigger a reading, wait for the result, and
> report it back to the user. If the intent here is to have the user write the
> mode (which triggers the one-shot reading), wait a bit, and then read the
> results, that doesn't make sense because standard userspace applications
> won't know that. Also, that would be unsynchronized - one has to read the
> CVRF bit in the mask/enable register to know if the reading is complete.
I think I oversimplified the one-shot mode here and you are right:
there should be a one-shot reading routine; the conversion time in
the configuration register also needs to be taken care of.
> The effort to do all this using CPU cycles would in most if not all cases
> outweigh any perceived power savings. As such, I just don't see the
> practical use case.
It really depends on the use case and how often the one-shot gets
triggered. For battery-powered devices, running in the continuous
mode does consume considerable power based on the measurement from
our power folks. If a system is running in a power sensitive mode,
while it still needs to occasionally check the inputs, it could be
a use case for one-shot mode, though it's purely a user decision.
> power-down mode effectively reinvents runtime power management (runtime
> suspend/resume support) and is thus simply unacceptable.
Similar to one-shot, if a system is in a low power mode where it
doesn't want to check the inputs anymore, I feel the user space
could at least make the decision to turn on/off the chips, I am
not quite sure if the generic runtime PM system already has this
kind of support though.
> I am open to help explore adding support for runtime power management
> to the hwmon subsystem, but that would be less than straightforward and
> require an actual use case to warrant the effort.
Is there any feasible solution from your point of view?
Thanks
Nicolin
>
> As such, NACK, sorry.
>
> Thanks,
> Guenter
>
> > Signed-off-by: Nicolin Chen
> > ---
> > drivers/hwmon/ina3221.c | 64 +
> > 1 file changed, 64 insertions(+)
> >
> > diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> > index d61688f04594..5218fd85506d 100644
> > --- a/drivers/hwmon/ina3221.c
> > +++ b/drivers/hwmon/ina3221.c
> > @@ -77,6 +77,28 @@ enum ina3221_channels {
> > INA3221_NUM_CHANNELS
> > };
> > +enum ina3221_modes {
> > + INA3221_MODE_POWERDOWN,
> > + INA3221_MODE_ONESHOT,
> > + INA3221_MODE_CONTINUOUS,
> > + INA3221_NUM_MODES,
> > +};
> > +
> > +static const char *ina3221_mode_names[INA3221_NUM_MODES] = {
> > + [INA3221_MODE_POWERDOWN] = "power-down",
> > + [INA3221_MODE_ONESHOT] = "one-shot",
> > + [INA3221_MODE_CONTINUOUS] = "continuous",
> > +};
> > +
> > +static const u16 ina3221_mode_val[] = {
> > + [INA3221_MODE_POWERDOWN] = INA3221_CONFIG_MODE_POWERDOWN,
> > + [INA3221_MODE_ONESHOT] = INA3221_CONFIG_MODE_SHUNT |
> > +INA3221_CONFIG_MODE_BUS,
> > + [INA3221_MODE_CONTINUOUS] = INA3221_CONFIG_MODE_CONTINUOUS |
> > + INA3221_CONFIG_MODE_SHUNT |
> > + INA3221_CONFIG_MODE_BUS,
> > +};
> > +
> > /**
> >* struct ina3221_input - channel input source specific information
> >* @label: label of channel input source
> > @@ -386,9 +408,51 @@ static const struct hwmon_ops ina3221_hwmon_ops = {
> > .write = ina3221_write,
> > };
> > +static int ina3221_mode_get_index(struct device *dev, unsigned int *index)
> > +{
> > + struct ina3221_data *ina = dev_get_drvdata(dev);
> > + u16 mode = ina->reg_config & INA3221_CONFIG_MODE_MASK;
> > +
> > + if (mode == INA3221_CONFIG_MODE_POWERDOWN)
> > + *index = INA3221_MODE_POWERDOWN;
> > + if (mode & INA3221_CONFIG_MODE_CONTINUOUS)
> > + *index = INA3221_MODE_CONTINUOUS;
> > + else
> > + *index = INA3221_MODE_ONESHOT;
> > +
> > + return 0;
> > +}
> > +
> > +static int ina3221_mode_set_index(struct device *dev, unsigned int index)
> > +{
> > + struct ina3221_data *ina = dev_get_drvdata(dev);
[PATCH 2/2] hwmon: (ina3221) Add operating mode support
The hwmon core now has a new optional mode interface. So this patch
just implements this mode support so that user space can check and
configure via sysfs node its operating modes: power-down, one-shot,
and continuous modes.
Signed-off-by: Nicolin Chen
---
drivers/hwmon/ina3221.c | 64 +
1 file changed, 64 insertions(+)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index d61688f04594..5218fd85506d 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -77,6 +77,28 @@ enum ina3221_channels {
INA3221_NUM_CHANNELS
};
+enum ina3221_modes {
+ INA3221_MODE_POWERDOWN,
+ INA3221_MODE_ONESHOT,
+ INA3221_MODE_CONTINUOUS,
+ INA3221_NUM_MODES,
+};
+
+static const char *ina3221_mode_names[INA3221_NUM_MODES] = {
+ [INA3221_MODE_POWERDOWN] = "power-down",
+ [INA3221_MODE_ONESHOT] = "one-shot",
+ [INA3221_MODE_CONTINUOUS] = "continuous",
+};
+
+static const u16 ina3221_mode_val[] = {
+ [INA3221_MODE_POWERDOWN] = INA3221_CONFIG_MODE_POWERDOWN,
+ [INA3221_MODE_ONESHOT] = INA3221_CONFIG_MODE_SHUNT |
+INA3221_CONFIG_MODE_BUS,
+ [INA3221_MODE_CONTINUOUS] = INA3221_CONFIG_MODE_CONTINUOUS |
+ INA3221_CONFIG_MODE_SHUNT |
+ INA3221_CONFIG_MODE_BUS,
+};
+
/**
* struct ina3221_input - channel input source specific information
* @label: label of channel input source
@@ -386,9 +408,51 @@ static const struct hwmon_ops ina3221_hwmon_ops = {
.write = ina3221_write,
};
+static int ina3221_mode_get_index(struct device *dev, unsigned int *index)
+{
+ struct ina3221_data *ina = dev_get_drvdata(dev);
+ u16 mode = ina->reg_config & INA3221_CONFIG_MODE_MASK;
+
+ if (mode == INA3221_CONFIG_MODE_POWERDOWN)
+ *index = INA3221_MODE_POWERDOWN;
+ if (mode & INA3221_CONFIG_MODE_CONTINUOUS)
+ *index = INA3221_MODE_CONTINUOUS;
+ else
+ *index = INA3221_MODE_ONESHOT;
+
+ return 0;
+}
+
+static int ina3221_mode_set_index(struct device *dev, unsigned int index)
+{
+ struct ina3221_data *ina = dev_get_drvdata(dev);
+ int ret;
+
+ ret = regmap_update_bits(ina->regmap, INA3221_CONFIG,
+INA3221_CONFIG_MODE_MASK,
+ina3221_mode_val[index]);
+ if (ret)
+ return ret;
+
+ /* Cache the latest config register value */
+ ret = regmap_read(ina->regmap, INA3221_CONFIG, >reg_config);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static const struct hwmon_mode ina3221_hwmon_mode = {
+ .names = ina3221_mode_names,
+ .list_size = INA3221_NUM_MODES,
+ .get_index = ina3221_mode_get_index,
+ .set_index = ina3221_mode_set_index,
+};
+
static const struct hwmon_chip_info ina3221_chip_info = {
.ops = _hwmon_ops,
.info = ina3221_info,
+ .mode = _hwmon_mode,
};
/* Extra attribute groups */
--
2.17.1
[PATCH 1/2] hwmon: (core) Add hwmon_mode structure and mode sysfs node
There are a few hwmon sensors support different operating modes,
for example, one-shot and continuous modes. So it's probably not
a bad idea to abstract a mode sysfs node as a common feature in
the hwmon core.
Right beside the hwmon device name, this patch adds a new sysfs
attribute named "mode" and "available_modes" for user to check
and configure the operating mode. For hwmon device drivers that
implemented the _with_info API, the change also adds an optional
hwmon_mode structure in hwmon_chip_info structure so that those
drivers can pass mode related information.
Signed-off-by: Nicolin Chen
---
Documentation/hwmon/sysfs-interface | 15 +
drivers/hwmon/hwmon.c | 87 ++---
include/linux/hwmon.h | 24
3 files changed, 119 insertions(+), 7 deletions(-)
diff --git a/Documentation/hwmon/sysfs-interface
b/Documentation/hwmon/sysfs-interface
index 2b9e1005d88b..48d6ca6b9bd4 100644
--- a/Documentation/hwmon/sysfs-interface
+++ b/Documentation/hwmon/sysfs-interface
@@ -92,6 +92,21 @@ name The chip name.
I2C devices get this attribute created automatically.
RO
+available_modes The available operating modes of the chip.
+ This should be short, lowercase string, not containing
+ whitespace, or the wildcard character '*'.
+ This attribute shows all the available of the operating modes,
+ for example, "power-down" "one-shot" and "continuous".
+ RO
+
+mode The current operating mode of the chip.
+ This should be short, lowercase string, not containing
+ whitespace, or the wildcard character '*'.
+ This attribute shows the current operating mode of the chip.
+ Writing a valid string from the list of available_modes will
+ configure the chip to the corresponding operating mode.
+ RW
+
update_intervalThe interval at which the chip will update readings.
Unit: millisecond
RW
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 975c95169884..5a33b616284b 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -72,25 +72,88 @@ name_show(struct device *dev, struct device_attribute
*attr, char *buf)
}
static DEVICE_ATTR_RO(name);
+static ssize_t available_modes_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct hwmon_device *hwdev = to_hwmon_device(dev);
+ const struct hwmon_chip_info *chip = hwdev->chip;
+ const struct hwmon_mode *mode = chip->mode;
+ int i;
+
+ for (i = 0; i < mode->list_size; i++)
+ snprintf(buf, PAGE_SIZE, "%s%s ", buf, mode->names[i]);
+
+ return snprintf(buf, PAGE_SIZE, "%s\n", buf);
+}
+static DEVICE_ATTR_RO(available_modes);
+
+static ssize_t mode_show(struct device *dev,
+struct device_attribute *attr, char *buf)
+{
+ struct hwmon_device *hwdev = to_hwmon_device(dev);
+ const struct hwmon_chip_info *chip = hwdev->chip;
+ const struct hwmon_mode *mode = chip->mode;
+ unsigned int index;
+ int ret;
+
+ ret = mode->get_index(dev, );
+ if (ret)
+ return ret;
+
+ return snprintf(buf, PAGE_SIZE, "%s\n", mode->names[index]);
+}
+
+static ssize_t mode_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct hwmon_device *hwdev = to_hwmon_device(dev);
+ const struct hwmon_chip_info *chip = hwdev->chip;
+ const struct hwmon_mode *mode = chip->mode;
+ const char **names = mode->names;
+ unsigned int index;
+ int ret;
+
+ /* Get the corresponding mode index */
+ for (index = 0; index < mode->list_size; index++) {
+ if (!strncmp(buf, names[index], strlen(names[index])))
+ break;
+ }
+
+ if (index >= mode->list_size)
+ return -EINVAL;
+
+ ret = mode->set_index(dev, index);
+ if (ret)
+ return ret;
+
+ return count;
+}
+static DEVICE_ATTR_RW(mode);
+
static struct attribute *hwmon_dev_attrs[] = {
- _attr_name.attr,
+ _attr_name.attr,/* index = 0 */
+ _attr_available_modes.attr, /* index = 1 */
+ _attr_mode.attr,/* index = 2 */
NULL
};
-static umode_t hwmon_dev_name_is_visible(struct kobject *kobj,
-struct attribute *attr, int n)
+static umode_t hwmon_dev_is_visible(struct kobject *kobj,
+ struct attribute *attr, int n)
{
struct device *dev = container_of(kobj, struct device, kobj);
+ struct hwmon_d
[PATCH 0/2] hwmon: Add operating mode support for core and ina3221
Add operating mode support in the core and ina3221. Nicolin Chen (2): hwmon: (core) Add hwmon_mode structure and mode sysfs node hwmon: (ina3221) Add operating mode support Documentation/hwmon/sysfs-interface | 15 + drivers/hwmon/hwmon.c | 87 ++--- drivers/hwmon/ina3221.c | 64 + include/linux/hwmon.h | 24 4 files changed, 183 insertions(+), 7 deletions(-) -- 2.17.1
[PATCH v2] hwmon: (core) Add trace events to _attr_show/store functions
Trace events are useful for people who collect data from the
Ftrace outputs. There're people who analyse the relationship
of cpufreq, thermal and hwmon (power/voltage/current) using
the convenient and timestamped Ftrace outputs, while unlike
cpufreq and thermal subsystems the hwmon does not have trace
events supported yet.
So this patch adds initial trace events for the hwmon core.
To call hwmon_attr_base() for aligned attr index numbers, it
also moves the function upward.
Ftrace outputs:
...: hwmon_attr_show_string: index=2, attr_name=in2_label, val=VDD_5V
...: hwmon_attr_show: index=2, attr_name=in2_input, val=5112
...: hwmon_attr_show: index=2, attr_name=curr2_input, val=440
Note that the _attr_show and _attr_store functions are tied
to the _with_info API. So a hwmon driver requiring the trace
events feature should use _with_info API to register a hwmon
device.
Signed-off-by: Nicolin Chen
---
Changelog
v1->v2:
* Added a descriptive reason for this change in the commit message
MAINTAINERS | 1 +
drivers/hwmon/hwmon.c| 27 ++
include/trace/events/hwmon.h | 71
3 files changed, 92 insertions(+), 7 deletions(-)
create mode 100644 include/trace/events/hwmon.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 1640b9faa75e..589b32405bf4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6461,6 +6461,7 @@ F:Documentation/devicetree/bindings/hwmon/
F: Documentation/hwmon/
F: drivers/hwmon/
F: include/linux/hwmon*.h
+F: include/trace/events/hwmon*.h
HARDWARE RANDOM NUMBER GENERATOR CORE
M: Matt Mackall
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index ac1cdf88840f..975c95169884 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -24,6 +24,9 @@
#include
#include
+#define CREATE_TRACE_POINTS
+#include
+
#define HWMON_ID_PREFIX "hwmon"
#define HWMON_ID_FORMAT HWMON_ID_PREFIX "%d"
@@ -171,6 +174,13 @@ static int hwmon_thermal_add_sensor(struct device *dev,
}
#endif /* IS_REACHABLE(CONFIG_THERMAL) && ... */
+static int hwmon_attr_base(enum hwmon_sensor_types type)
+{
+ if (type == hwmon_in)
+ return 0;
+ return 1;
+}
+
/* sysfs attribute management */
static ssize_t hwmon_attr_show(struct device *dev,
@@ -185,6 +195,9 @@ static ssize_t hwmon_attr_show(struct device *dev,
if (ret < 0)
return ret;
+ trace_hwmon_attr_show(hattr->index + hwmon_attr_base(hattr->type),
+ hattr->name, val);
+
return sprintf(buf, "%ld\n", val);
}
@@ -193,6 +206,7 @@ static ssize_t hwmon_attr_show_string(struct device *dev,
char *buf)
{
struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
+ enum hwmon_sensor_types type = hattr->type;
const char *s;
int ret;
@@ -201,6 +215,9 @@ static ssize_t hwmon_attr_show_string(struct device *dev,
if (ret < 0)
return ret;
+ trace_hwmon_attr_show_string(hattr->index + hwmon_attr_base(type),
+hattr->name, s);
+
return sprintf(buf, "%s\n", s);
}
@@ -221,16 +238,12 @@ static ssize_t hwmon_attr_store(struct device *dev,
if (ret < 0)
return ret;
+ trace_hwmon_attr_store(hattr->index + hwmon_attr_base(hattr->type),
+ hattr->name, val);
+
return count;
}
-static int hwmon_attr_base(enum hwmon_sensor_types type)
-{
- if (type == hwmon_in)
- return 0;
- return 1;
-}
-
static bool is_string_attr(enum hwmon_sensor_types type, u32 attr)
{
return (type == hwmon_temp && attr == hwmon_temp_label) ||
diff --git a/include/trace/events/hwmon.h b/include/trace/events/hwmon.h
new file mode 100644
index ..d7a1d0ffb679
--- /dev/null
+++ b/include/trace/events/hwmon.h
@@ -0,0 +1,71 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM hwmon
+
+#if !defined(_TRACE_HWMON_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_HWMON_H
+
+#include
+
+DECLARE_EVENT_CLASS(hwmon_attr_class,
+
+ TP_PROTO(int index, const char *attr_name, long val),
+
+ TP_ARGS(index, attr_name, val),
+
+ TP_STRUCT__entry(
+ __field(int, index)
+ __string(attr_name, attr_name)
+ __field(long, val)
+ ),
+
+ TP_fast_assign(
+ __entry->index = index;
+ __assign_str(attr_name, attr_name);
+ __entry->val = val;
+ ),
+
+ TP_printk("index=%d, attr_name=%s, val=%ld",
+ __entry->index, __get_str(attr_name), __entry->val)
+);
+
+DEFINE_EVENT(hwmon_attr_class, hwmon_attr_show,
+
+ TP_PROTO(int index, const char *attr_name, long val),
+
+
Re: [PATCH] hwmon: (core) Add trace events to _attr_show/store functions
On Tue, Oct 09, 2018 at 03:49:45PM -0400, Steven Rostedt wrote:
> > > > Trace events are useful for people who collect data from the
> > > > ftrace output. This patch adds initial trace events for the
> > > > hwmon core. To call hwmon_attr_base() for aligned attr index
> > > > numbers, this patch also moves the function upward.
> > > >
> > > > Ftrace outputs:
> > > > ...: hwmon_attr_show_string: index=2, attr_name=in2_label, val=VDD_5V
> > > > ...: hwmon_attr_show: index=2, attr_name=in2_input, val=5112
> > > > ...: hwmon_attr_show: index=2, attr_name=curr2_input, val=440
> > > >
> > > > Note that the _attr_show and _attr_store functions are tied
> > > > to the _with_info API. So a hwmon driver requiring the trace
> > > > events feature should use _with_info API to register a hwmon
> > > > device.
> > >
> > > Hmm, this doesn't really explain why these trace events are needed.
> > > They look to be attached to sysfs reads. What's the purpose of tracing
> > > these?
> >
> > Our power folks analyse Ftrace outputs of cpufreq, thermal and
> > hwmon (power/voltage/current) so as to get the relationship of
> > them. The reason why we specifically need trace events is that
> > it's convenient and timestamped, and because both cpufreq and
> > thermal already have trace events.
> >
> > I could add this to make the commit message more convincing if
> > you'd prefer that.
>
> I'm not against the patch, but yes, having this in the change log is
> helpful.
I will add it in v2.
> > > > @@ -193,6 +206,7 @@ static ssize_t hwmon_attr_show_string(struct device
> > > > *dev,
> > > > char *buf)
> > > > {
> > > > struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
> > > > + enum hwmon_sensor_types type = hattr->type;
> > > > const char *s;
> > > > int ret;
> > > >
> > > > @@ -201,6 +215,9 @@ static ssize_t hwmon_attr_show_string(struct device
> > > > *dev,
> > > > if (ret < 0)
> > > > return ret;
> > > >
> > > > + trace_hwmon_attr_show_string(hattr->index +
> > > > hwmon_attr_base(type),
> > >
> > > Also, the other to tracepoints use hattr->type, here you create a
> > > separate variable. Is that just to keep within the 80 char limit?
> >
> > Yes. It looks clearer to me than wrapping the line here, since
> > complier usually optimize the local variable away.
>
> Or you can just break the 80 character limit ;-)
Personally a checkpatch warning bugs me more than having an extra
line :)
Thank you
Nicolin
Re: [PATCH] hwmon: (core) Add trace events to _attr_show/store functions
On Tue, Oct 09, 2018 at 02:57:21PM -0400, Steven Rostedt wrote:
> > Trace events are useful for people who collect data from the
> > ftrace output. This patch adds initial trace events for the
> > hwmon core. To call hwmon_attr_base() for aligned attr index
> > numbers, this patch also moves the function upward.
> >
> > Ftrace outputs:
> > ...: hwmon_attr_show_string: index=2, attr_name=in2_label, val=VDD_5V
> > ...: hwmon_attr_show: index=2, attr_name=in2_input, val=5112
> > ...: hwmon_attr_show: index=2, attr_name=curr2_input, val=440
> >
> > Note that the _attr_show and _attr_store functions are tied
> > to the _with_info API. So a hwmon driver requiring the trace
> > events feature should use _with_info API to register a hwmon
> > device.
>
> Hmm, this doesn't really explain why these trace events are needed.
> They look to be attached to sysfs reads. What's the purpose of tracing
> these?
Our power folks analyse Ftrace outputs of cpufreq, thermal and
hwmon (power/voltage/current) so as to get the relationship of
them. The reason why we specifically need trace events is that
it's convenient and timestamped, and because both cpufreq and
thermal already have trace events.
I could add this to make the commit message more convincing if
you'd prefer that.
> > @@ -193,6 +206,7 @@ static ssize_t hwmon_attr_show_string(struct device
> > *dev,
> > char *buf)
> > {
> > struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
> > + enum hwmon_sensor_types type = hattr->type;
> > const char *s;
> > int ret;
> >
> > @@ -201,6 +215,9 @@ static ssize_t hwmon_attr_show_string(struct device
> > *dev,
> > if (ret < 0)
> > return ret;
> >
> > + trace_hwmon_attr_show_string(hattr->index + hwmon_attr_base(type),
>
> Also, the other to tracepoints use hattr->type, here you create a
> separate variable. Is that just to keep within the 80 char limit?
Yes. It looks clearer to me than wrapping the line here, since
complier usually optimize the local variable away.
Thanks
Nicolin
[PATCH] hwmon: (core) Add trace events to _attr_show/store functions
Trace events are useful for people who collect data from the
ftrace output. This patch adds initial trace events for the
hwmon core. To call hwmon_attr_base() for aligned attr index
numbers, this patch also moves the function upward.
Ftrace outputs:
...: hwmon_attr_show_string: index=2, attr_name=in2_label, val=VDD_5V
...: hwmon_attr_show: index=2, attr_name=in2_input, val=5112
...: hwmon_attr_show: index=2, attr_name=curr2_input, val=440
Note that the _attr_show and _attr_store functions are tied
to the _with_info API. So a hwmon driver requiring the trace
events feature should use _with_info API to register a hwmon
device.
Signed-off-by: Nicolin Chen
---
MAINTAINERS | 1 +
drivers/hwmon/hwmon.c| 27 ++
include/trace/events/hwmon.h | 71
3 files changed, 92 insertions(+), 7 deletions(-)
create mode 100644 include/trace/events/hwmon.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 1640b9faa75e..589b32405bf4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6461,6 +6461,7 @@ F:Documentation/devicetree/bindings/hwmon/
F: Documentation/hwmon/
F: drivers/hwmon/
F: include/linux/hwmon*.h
+F: include/trace/events/hwmon*.h
HARDWARE RANDOM NUMBER GENERATOR CORE
M: Matt Mackall
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index ac1cdf88840f..975c95169884 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -24,6 +24,9 @@
#include
#include
+#define CREATE_TRACE_POINTS
+#include
+
#define HWMON_ID_PREFIX "hwmon"
#define HWMON_ID_FORMAT HWMON_ID_PREFIX "%d"
@@ -171,6 +174,13 @@ static int hwmon_thermal_add_sensor(struct device *dev,
}
#endif /* IS_REACHABLE(CONFIG_THERMAL) && ... */
+static int hwmon_attr_base(enum hwmon_sensor_types type)
+{
+ if (type == hwmon_in)
+ return 0;
+ return 1;
+}
+
/* sysfs attribute management */
static ssize_t hwmon_attr_show(struct device *dev,
@@ -185,6 +195,9 @@ static ssize_t hwmon_attr_show(struct device *dev,
if (ret < 0)
return ret;
+ trace_hwmon_attr_show(hattr->index + hwmon_attr_base(hattr->type),
+ hattr->name, val);
+
return sprintf(buf, "%ld\n", val);
}
@@ -193,6 +206,7 @@ static ssize_t hwmon_attr_show_string(struct device *dev,
char *buf)
{
struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
+ enum hwmon_sensor_types type = hattr->type;
const char *s;
int ret;
@@ -201,6 +215,9 @@ static ssize_t hwmon_attr_show_string(struct device *dev,
if (ret < 0)
return ret;
+ trace_hwmon_attr_show_string(hattr->index + hwmon_attr_base(type),
+hattr->name, s);
+
return sprintf(buf, "%s\n", s);
}
@@ -221,16 +238,12 @@ static ssize_t hwmon_attr_store(struct device *dev,
if (ret < 0)
return ret;
+ trace_hwmon_attr_store(hattr->index + hwmon_attr_base(hattr->type),
+ hattr->name, val);
+
return count;
}
-static int hwmon_attr_base(enum hwmon_sensor_types type)
-{
- if (type == hwmon_in)
- return 0;
- return 1;
-}
-
static bool is_string_attr(enum hwmon_sensor_types type, u32 attr)
{
return (type == hwmon_temp && attr == hwmon_temp_label) ||
diff --git a/include/trace/events/hwmon.h b/include/trace/events/hwmon.h
new file mode 100644
index ..d7a1d0ffb679
--- /dev/null
+++ b/include/trace/events/hwmon.h
@@ -0,0 +1,71 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM hwmon
+
+#if !defined(_TRACE_HWMON_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_HWMON_H
+
+#include
+
+DECLARE_EVENT_CLASS(hwmon_attr_class,
+
+ TP_PROTO(int index, const char *attr_name, long val),
+
+ TP_ARGS(index, attr_name, val),
+
+ TP_STRUCT__entry(
+ __field(int, index)
+ __string(attr_name, attr_name)
+ __field(long, val)
+ ),
+
+ TP_fast_assign(
+ __entry->index = index;
+ __assign_str(attr_name, attr_name);
+ __entry->val = val;
+ ),
+
+ TP_printk("index=%d, attr_name=%s, val=%ld",
+ __entry->index, __get_str(attr_name), __entry->val)
+);
+
+DEFINE_EVENT(hwmon_attr_class, hwmon_attr_show,
+
+ TP_PROTO(int index, const char *attr_name, long val),
+
+ TP_ARGS(index, attr_name, val)
+);
+
+DEFINE_EVENT(hwmon_attr_class, hwmon_attr_store,
+
+ TP_PROTO(int index, const char *attr_name, long val),
+
+ TP_ARGS(index, attr_name, val)
+);
+
+TRACE_EVENT(hwmon_attr_show_string,
+
+ TP_PROTO(int index, const char *attr_name, const char *s),
+
+ TP_ARGS(index,
[PATCH v2] hwmon: (ina3221) Validate shunt resistor value from DT
The input->shunt_resistor is int type while the value from DT is
unsigned int. Meanwhile, a divide-by-zero error would happen if
the value is 0. So this patch just simply validates the value.
Signed-off-by: Nicolin Chen
---
Changelog
v1->v2:
* Validate the value and error out
drivers/hwmon/ina3221.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index c3a497aed345..4f3ed24efe8e 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -536,8 +536,14 @@ static int ina3221_probe_child_from_dt(struct device *dev,
of_property_read_string(child, "label", >label);
/* Overwrite default shunt resistor value optionally */
- if (!of_property_read_u32(child, "shunt-resistor-micro-ohms", ))
+ if (!of_property_read_u32(child, "shunt-resistor-micro-ohms", )) {
+ if (val < 1 || val > INT_MAX) {
+ dev_err(dev, "invalid shunt resistor value %u of %s\n",
+ val, child->name);
+ return -EINVAL;
+ }
input->shunt_resistor = val;
+ }
return 0;
}
--
2.17.1
[PATCH v2] hwmon: (ina3221) Use _info API to register hwmon device
The hwmon core has a newer API which abstracts most of common
things in the core so as to simplify the hwmon device drivers.
This patch implements this _info API to ina3221 hwmon driver.
It also reduces the binary size:
textdata bss dec hex filename
51141712 068261aaa drivers/hwmon/ina3221_before.o
4456 440 048961320 drivers/hwmon/ina3221.o
Signed-off-by: Nicolin Chen
---
Changelog
v1->v2:
* Added a binary size comparison in commit message
* Dropped unnecessary checks
* Dropped misleading ID conversion in the comments
* Added two comments to indicate 0-align channel indexes
* Used "fall through" instead of "fallthrough"
drivers/hwmon/ina3221.c | 520 ++--
1 file changed, 236 insertions(+), 284 deletions(-)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index ce8f0a8c4982..973fc4af0ddf 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -77,21 +77,6 @@ enum ina3221_channels {
INA3221_NUM_CHANNELS
};
-static const unsigned int register_channel[] = {
- [INA3221_BUS1] = INA3221_CHANNEL1,
- [INA3221_BUS2] = INA3221_CHANNEL2,
- [INA3221_BUS3] = INA3221_CHANNEL3,
- [INA3221_SHUNT1] = INA3221_CHANNEL1,
- [INA3221_SHUNT2] = INA3221_CHANNEL2,
- [INA3221_SHUNT3] = INA3221_CHANNEL3,
- [INA3221_CRIT1] = INA3221_CHANNEL1,
- [INA3221_CRIT2] = INA3221_CHANNEL2,
- [INA3221_CRIT3] = INA3221_CHANNEL3,
- [INA3221_WARN1] = INA3221_CHANNEL1,
- [INA3221_WARN2] = INA3221_CHANNEL2,
- [INA3221_WARN3] = INA3221_CHANNEL3,
-};
-
/**
* struct ina3221_input - channel input source specific information
* @label: label of channel input source
@@ -123,58 +108,6 @@ static inline bool ina3221_is_enabled(struct ina3221_data
*ina, int channel)
return ina->reg_config & INA3221_CONFIG_CHx_EN(channel);
}
-static ssize_t ina3221_show_label(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
- struct ina3221_data *ina = dev_get_drvdata(dev);
- unsigned int channel = sd_attr->index;
- struct ina3221_input *input = >inputs[channel];
-
- return snprintf(buf, PAGE_SIZE, "%s\n", input->label);
-}
-
-static ssize_t ina3221_show_enable(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
- struct ina3221_data *ina = dev_get_drvdata(dev);
- unsigned int channel = sd_attr->index;
-
- return snprintf(buf, PAGE_SIZE, "%d\n",
- ina3221_is_enabled(ina, channel));
-}
-
-static ssize_t ina3221_set_enable(struct device *dev,
- struct device_attribute *attr,
- const char *buf, size_t count)
-{
- struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
- struct ina3221_data *ina = dev_get_drvdata(dev);
- unsigned int channel = sd_attr->index;
- u16 config, mask = INA3221_CONFIG_CHx_EN(channel);
- bool enable;
- int ret;
-
- ret = kstrtobool(buf, );
- if (ret)
- return ret;
-
- config = enable ? mask : 0;
-
- /* Enable or disable the channel */
- ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, config);
- if (ret)
- return ret;
-
- /* Cache the latest config register value */
- ret = regmap_read(ina->regmap, INA3221_CONFIG, >reg_config);
- if (ret)
- return ret;
-
- return count;
-}
-
static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
int *val)
{
@@ -190,94 +123,104 @@ static int ina3221_read_value(struct ina3221_data *ina,
unsigned int reg,
return 0;
}
-static ssize_t ina3221_show_bus_voltage(struct device *dev,
- struct device_attribute *attr,
- char *buf)
+static const u8 ina3221_in_reg[] = {
+ INA3221_BUS1,
+ INA3221_BUS2,
+ INA3221_BUS3,
+ INA3221_SHUNT1,
+ INA3221_SHUNT2,
+ INA3221_SHUNT3,
+};
+
+static int ina3221_read_in(struct device *dev, u32 attr, int channel, long
*val)
{
- struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
+ const bool is_shunt = channel > INA3221_CHANNEL3;
struct ina3221_data *ina = dev_get_drvdata(dev);
- unsigned int reg = sd_attr->index;
- unsigned int channel = register_channel[reg];
- int val, voltage_mv, ret;
+ u8 reg = ina3221_in_reg[channel];
+ int regval, ret;
- /* No data for read-only attribute if channel is disabled */
- if (!a
[PATCH] hwmon: (ina3221) Clamp shunt resistor value from DT
The input->shunt_resistor is int type while the value from DT is
unsigned int. Meanwhile, a divide-by-zero error would happen if
the value is 0. So this patch just simply clamps the value.
Signed-off-by: Nicolin Chen
---
drivers/hwmon/ina3221.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index c3a497aed345..ce8f0a8c4982 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -536,8 +536,10 @@ static int ina3221_probe_child_from_dt(struct device *dev,
of_property_read_string(child, "label", >label);
/* Overwrite default shunt resistor value optionally */
- if (!of_property_read_u32(child, "shunt-resistor-micro-ohms", ))
+ if (!of_property_read_u32(child, "shunt-resistor-micro-ohms", )) {
+ val = clamp_val(val, 1, INT_MAX);
input->shunt_resistor = val;
+ }
return 0;
}
--
2.17.1
[PATCH 1/2] hwmon: (core) Add hwmon_in_enable attribute
According to hwmon ABI, in%d_enable is a sysfs interface that
allows user space to enable and disable the input sensor. So
this patch just simply adds the attribute to the list.
Signed-off-by: Nicolin Chen
---
drivers/hwmon/hwmon.c | 1 +
include/linux/hwmon.h | 2 ++
2 files changed, 3 insertions(+)
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 33d51281272b..ac1cdf88840f 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -356,6 +356,7 @@ static const char * const hwmon_in_attr_templates[] = {
[hwmon_in_max_alarm] = "in%d_max_alarm",
[hwmon_in_lcrit_alarm] = "in%d_lcrit_alarm",
[hwmon_in_crit_alarm] = "in%d_crit_alarm",
+ [hwmon_in_enable] = "in%d_enable",
};
static const char * const hwmon_curr_attr_templates[] = {
diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 9493d4a388db..99e0c1b0b5fb 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -118,6 +118,7 @@ enum hwmon_in_attributes {
hwmon_in_max_alarm,
hwmon_in_lcrit_alarm,
hwmon_in_crit_alarm,
+ hwmon_in_enable,
};
#define HWMON_I_INPUT BIT(hwmon_in_input)
@@ -135,6 +136,7 @@ enum hwmon_in_attributes {
#define HWMON_I_MAX_ALARM BIT(hwmon_in_max_alarm)
#define HWMON_I_LCRIT_ALARMBIT(hwmon_in_lcrit_alarm)
#define HWMON_I_CRIT_ALARM BIT(hwmon_in_crit_alarm)
+#define HWMON_I_ENABLE BIT(hwmon_in_enable)
enum hwmon_curr_attributes {
hwmon_curr_input,
--
2.17.1
[PATCH 2/2] hwmon: (ina3221) Use _info API to register hwmon device
The hwmon core has a newer API which abstracts most of common
things in the core so as to simplify the hwmon device drivers.
This patch implements this _info API to ina3221 hwmon driver.
Signed-off-by: Nicolin Chen
---
drivers/hwmon/ina3221.c | 528 +++-
1 file changed, 245 insertions(+), 283 deletions(-)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index c3a497aed345..08cc0b4bc0ba 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -77,21 +77,6 @@ enum ina3221_channels {
INA3221_NUM_CHANNELS
};
-static const unsigned int register_channel[] = {
- [INA3221_BUS1] = INA3221_CHANNEL1,
- [INA3221_BUS2] = INA3221_CHANNEL2,
- [INA3221_BUS3] = INA3221_CHANNEL3,
- [INA3221_SHUNT1] = INA3221_CHANNEL1,
- [INA3221_SHUNT2] = INA3221_CHANNEL2,
- [INA3221_SHUNT3] = INA3221_CHANNEL3,
- [INA3221_CRIT1] = INA3221_CHANNEL1,
- [INA3221_CRIT2] = INA3221_CHANNEL2,
- [INA3221_CRIT3] = INA3221_CHANNEL3,
- [INA3221_WARN1] = INA3221_CHANNEL1,
- [INA3221_WARN2] = INA3221_CHANNEL2,
- [INA3221_WARN3] = INA3221_CHANNEL3,
-};
-
/**
* struct ina3221_input - channel input source specific information
* @label: label of channel input source
@@ -123,58 +108,6 @@ static inline bool ina3221_is_enabled(struct ina3221_data
*ina, int channel)
return ina->reg_config & INA3221_CONFIG_CHx_EN(channel);
}
-static ssize_t ina3221_show_label(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
- struct ina3221_data *ina = dev_get_drvdata(dev);
- unsigned int channel = sd_attr->index;
- struct ina3221_input *input = >inputs[channel];
-
- return snprintf(buf, PAGE_SIZE, "%s\n", input->label);
-}
-
-static ssize_t ina3221_show_enable(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
- struct ina3221_data *ina = dev_get_drvdata(dev);
- unsigned int channel = sd_attr->index;
-
- return snprintf(buf, PAGE_SIZE, "%d\n",
- ina3221_is_enabled(ina, channel));
-}
-
-static ssize_t ina3221_set_enable(struct device *dev,
- struct device_attribute *attr,
- const char *buf, size_t count)
-{
- struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
- struct ina3221_data *ina = dev_get_drvdata(dev);
- unsigned int channel = sd_attr->index;
- u16 config, mask = INA3221_CONFIG_CHx_EN(channel);
- bool enable;
- int ret;
-
- ret = kstrtobool(buf, );
- if (ret)
- return ret;
-
- config = enable ? mask : 0;
-
- /* Enable or disable the channel */
- ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, config);
- if (ret)
- return ret;
-
- /* Cache the latest config register value */
- ret = regmap_read(ina->regmap, INA3221_CONFIG, >reg_config);
- if (ret)
- return ret;
-
- return count;
-}
-
static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
int *val)
{
@@ -190,94 +123,107 @@ static int ina3221_read_value(struct ina3221_data *ina,
unsigned int reg,
return 0;
}
-static ssize_t ina3221_show_bus_voltage(struct device *dev,
- struct device_attribute *attr,
- char *buf)
+static const u8 ina3221_in_reg[] = {
+ INA3221_BUS1,
+ INA3221_BUS2,
+ INA3221_BUS3,
+ INA3221_SHUNT1,
+ INA3221_SHUNT2,
+ INA3221_SHUNT3,
+};
+
+static int ina3221_read_in(struct device *dev, u32 attr, int channel, long
*val)
{
- struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
+ const bool is_shunt = channel > INA3221_CHANNEL3;
struct ina3221_data *ina = dev_get_drvdata(dev);
- unsigned int reg = sd_attr->index;
- unsigned int channel = register_channel[reg];
- int val, voltage_mv, ret;
+ u8 reg = ina3221_in_reg[channel];
+ int regval, ret;
- /* No data for read-only attribute if channel is disabled */
- if (!attr->store && !ina3221_is_enabled(ina, channel))
- return -ENODATA;
+ /* Translate shunt channel ID to sensor channel ID: 4->1, 5->2, 6->3 */
+ channel %= INA3221_NUM_CHANNELS;
- ret = ina3221_read_value(ina, reg, );
- if (ret)
- return ret;
+ switch (attr) {
+ case hwmon_in_input:
+ if (!ina3221_is_enabled(ina, channel))
+ return -ENODATA;
- voltage_mv = val * 8;
[PATCH 0/2] hwmon: (ina3221) Apply _info API
This series of patches applies _info API of hwmon core to the ina3221 hwmon driver. Nicolin Chen (2): hwmon: (core) Add hwmon_in_enable attribute hwmon: (ina3221) Use _info API to register hwmon device drivers/hwmon/hwmon.c | 1 + drivers/hwmon/ina3221.c | 528 +++- include/linux/hwmon.h | 2 + 3 files changed, 248 insertions(+), 283 deletions(-) -- 2.17.1
Re: hwmon: trace event support?
Hello, On Thu, Oct 04, 2018 at 09:48:02AM -0700, Guenter Roeck wrote: > > > I would not object to adding trace support into the hwmon subsystem. > > > However, it should be tied to the new API. I would resist patches > > > adding trace support to individual hwmon drivers unless the new API > > > is used and additional driver specific trace support is warranted. > > > > Yes, my idea is to implement it with the _info API inside the hwmon > > core. What do you think about the mentioned solution? Would you be > > in favor of a polling work queue? > > > > "--. Similar to tz->poll_queue in thermal_core, hwmon core > > could also have a work queue polling the registered sensor inputs > > (by default disabled; enabled only if users configure poll_delay) > > so that the power data can be generated to Ftrace outputs as well." > > > > I am not really in favor of it. This goes well beyond tracing. Tracing > by its nature should be non-invasive and impact the system as little as > possible. Adding a thread which polls thermal sensors, which are often > connected with a slow i2c interface or even blocking, is quite invasive. > > I don't mind adding tracing support to trace sensor access. Adding code > to poll thermal sensors on a regular basis is a completely different > beast. I am not convinced that this should really be done in the kernel. > The same could be accomplished with a simple loop from userspace. I ain't 100% convinced either. I think at this point we can just insert a trace event to the hwmon_attr_show(), unless there is a substantial polling queue in the hwmon core as thermal_core has, although I am not sure what would be a legit reason to add one. > while true; do > cat /sys/class/hwmon/hwmon1/temp1_input > sleep 1 > done The power/perf folks were asking about something hands-free, as neither thermal nor cpufreq requires extra readings or polling, but I feel this should work for them too, reluctantly though. > ... and you could actually trace those accesses in the kernel. > > Now, if the problem is added overhead due to requiring a sysfs access > for each sensor read, we can discuss introducing a different and more > efficient user-space ABI (such as adding a hwmon->iio bridge). > That would however be a different discussion. Yea, that's beyond the topic yet it sounds more interesting for certain people I guess, considering the fact that there are two ina2xx drivers in both hwmon and iio subsystems. > > > Note that this also applies to hwmon drivers registering through > > > thermal. The thermal subsystem calls the _info API but misuses it > > > to avoid a warning generated when using the old API. Of course, > > > I have no influence over the hwmon code in the thermal subsystem, > > > so whatever is done there is essentially wild-wild-west. > > > > I saw they have some obvious code in the hwmon core. If you want, > > we can keep the polling work queue and trace events away from it, > > which sounds plausible to me considering that thermal subsystem > > has its own polling work queue and trace events for sensor data. > > The code in the hwmon core is different. I am referring to hwmon code > in the thermal core. I see, though I can't foresee a conflict if we just add a trace event in the hwmon_attr_show(). And it seems, at least now, it passes a NULL chip pointer via the _info API. Thank you Nicolin
Re: hwmon: trace event support?
Thanks for the quick response. On Wed, Oct 03, 2018 at 05:42:23PM -0700, Guenter Roeck wrote: > > Ftrace data. Similar to tz->poll_queue in thermal_core, hwmon core > > could also have a work queue polling the registered sensor inputs > > (by default disabled; enabled only if users configure poll_delay) > > so that the power data can be generated to Ftrace outputs as well. > > > > To add this, the hwmon core needs an interface that can get sensor > > inputs as drivers like ina3221 don't report any values back to the > > core but directly expose them via sysfs ABI nodes. > > > > I noticed the hwmon_device_register_with_info() could be actually > > a good API to use since it has defined different sensor types and > > more importantly the ops->read() interface, but the ina3221 driver > > is very compact that there would be very little gain from this API > > at this moment. However, maybe having trace event support would be > > "very little gain" is a false assumption. Its size would most likely > be reduced by 20% or more, mostly because all the sysfs handling > is done by the core if one uses the _info API. Didn't intend to deny the improvement; probably should have used a more positive word instead of "little" :) > > a good reason to apply this API. > > > > What do you think about it? Any concern or better solution? > > I would not object to adding trace support into the hwmon subsystem. > However, it should be tied to the new API. I would resist patches > adding trace support to individual hwmon drivers unless the new API > is used and additional driver specific trace support is warranted. Yes, my idea is to implement it with the _info API inside the hwmon core. What do you think about the mentioned solution? Would you be in favor of a polling work queue? "--. Similar to tz->poll_queue in thermal_core, hwmon core could also have a work queue polling the registered sensor inputs (by default disabled; enabled only if users configure poll_delay) so that the power data can be generated to Ftrace outputs as well." > Note that this also applies to hwmon drivers registering through > thermal. The thermal subsystem calls the _info API but misuses it > to avoid a warning generated when using the old API. Of course, > I have no influence over the hwmon code in the thermal subsystem, > so whatever is done there is essentially wild-wild-west. I saw they have some obvious code in the hwmon core. If you want, we can keep the polling work queue and trace events away from it, which sounds plausible to me considering that thermal subsystem has its own polling work queue and trace events for sensor data. Thank you Nicolin
hwmon: trace event support?
Hello Guenter, I am thinking about a possibility of adding a trace event support in hwmon subsystem. Ftrace is pretty useful and widely applied to different subsystems already while hwmon doesn't seem to have one yet. I know some power/perf folks who rely on the trace events to analyse relationships among cpufreq <-> thermal <-> power: usually what they do is enabling trace events, then letting the system run for a while, and finally collecting all data from Ftrace outputs. Both cpufreq and thermal have trace events; but for power data, it seems that they have to go through sysfs nodes, which is difficult for them to get data at the same timestamps corresponding to those Ftrace data. Similar to tz->poll_queue in thermal_core, hwmon core could also have a work queue polling the registered sensor inputs (by default disabled; enabled only if users configure poll_delay) so that the power data can be generated to Ftrace outputs as well. To add this, the hwmon core needs an interface that can get sensor inputs as drivers like ina3221 don't report any values back to the core but directly expose them via sysfs ABI nodes. I noticed the hwmon_device_register_with_info() could be actually a good API to use since it has defined different sensor types and more importantly the ops->read() interface, but the ina3221 driver is very compact that there would be very little gain from this API at this moment. However, maybe having trace event support would be a good reason to apply this API. What do you think about it? Any concern or better solution? Thank you Nicolin
