[PATCH v4 0/4] power: supply: core: extend with new properties

[PATCH v4 0/4] power: supply: core: extend with new properties

[Michał Mirosław]

This series extend power supply class core with additional properties
for measurements of power supply input and output power.

v4 is a rebase on top of recently applied first part of v3, including
patch 1 workaround for gcc and clang bugs.

Michał Mirosław (4):
  power: supply: core: tabularize HWMON temperature labels
  power: supply: core: add input voltage/current measurements
  power: supply: core: add output voltage measurements
  power: supply: core: document measurement points

 Documentation/power/power_supply_class.rst |   6 +
 drivers/power/supply/power_supply_hwmon.c  | 142 ++++++++++++++++++++-
 drivers/power/supply/power_supply_sysfs.c  |   5 +
 include/linux/power_supply.h               |   5 +
 4 files changed, 152 insertions(+), 6 deletions(-)

-- 
2.20.1

[PATCH v4 1/4] power: supply: core: tabularize HWMON temperature labels

[Michał Mirosław]

Rework power_supply_hwmon_read_string() to check it's parameters.
This allows to extend it later with labels for other types of
measurements.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
v2: split from fix temperature labels
v3: remove power_supply_hwmon_read_string() parameter checks
    as it is internal API (suggested by Guenter Roeck)
v4: remove unreachable() as it triggers compiler bugs
---
 drivers/power/supply/power_supply_hwmon.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
index af72e5693f65..f5d538485aaa 100644
--- a/drivers/power/supply/power_supply_hwmon.c
+++ b/drivers/power/supply/power_supply_hwmon.c
@@ -13,6 +13,11 @@ struct power_supply_hwmon {
 	unsigned long *props;
 };
 
+static const char *const ps_temp_label[] = {
+	"temp",
+	"ambient temp",
+};
+
 static int power_supply_hwmon_in_to_property(u32 attr)
 {
 	switch (attr) {
@@ -180,7 +185,20 @@ static int power_supply_hwmon_read_string(struct device *dev,
 					  u32 attr, int channel,
 					  const char **str)
 {
-	*str = channel ? "temp ambient" : "temp";
+	switch (type) {
+	case hwmon_temp:
+		*str = ps_temp_label[channel];
+		break;
+	default:
+		/* unreachable, but see:
+		 * gcc bug #51513 [1] and clang bug #978 [2]
+		 *
+		 * [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51513
+		 * [2] https://github.com/ClangBuiltLinux/linux/issues/978
+		 */
+		break;
+	}
+
 	return 0;
 }
 
-- 
2.20.1

[PATCH v4 2/4] power: supply: core: add input voltage/current measurements

[Michał Mirosław]

Introduce input voltage and current limits and measurements.
This makes room for e.g. VBUS measurements in USB chargers.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
v2: add parameter checking in power_supply_hwmon_read_string()
v3: remove power_supply_hwmon_read_string() parameter checks
    as it is internal API (suggested by Guenter Roeck)
---
 drivers/power/supply/power_supply_hwmon.c | 97 +++++++++++++++++++++--
 drivers/power/supply/power_supply_sysfs.c |  2 +
 include/linux/power_supply.h              |  2 +
 3 files changed, 96 insertions(+), 5 deletions(-)

diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
index f5d538485aaa..3863b2a73ecf 100644
--- a/drivers/power/supply/power_supply_hwmon.c
+++ b/drivers/power/supply/power_supply_hwmon.c
@@ -13,12 +13,17 @@ struct power_supply_hwmon {
 	unsigned long *props;
 };
 
+static const char *const ps_input_label[] = {
+	"battery",
+	"external source",
+};
+
 static const char *const ps_temp_label[] = {
 	"temp",
 	"ambient temp",
 };
 
-static int power_supply_hwmon_in_to_property(u32 attr)
+static int power_supply_hwmon_in0_to_property(u32 attr)
 {
 	switch (attr) {
 	case hwmon_in_average:
@@ -34,7 +39,31 @@ static int power_supply_hwmon_in_to_property(u32 attr)
 	}
 }
 
-static int power_supply_hwmon_curr_to_property(u32 attr)
+static int power_supply_hwmon_in1_to_property(u32 attr)
+{
+	switch (attr) {
+	case hwmon_in_max:
+		return POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT;
+	case hwmon_in_input:
+		return POWER_SUPPLY_PROP_INPUT_VOLTAGE_NOW;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int power_supply_hwmon_in_to_property(u32 attr, int channel)
+{
+	switch (channel) {
+	case 0:
+		return power_supply_hwmon_in0_to_property(attr);
+	case 1:
+		return power_supply_hwmon_in1_to_property(attr);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int power_supply_hwmon_curr0_to_property(u32 attr)
 {
 	switch (attr) {
 	case hwmon_curr_average:
@@ -48,6 +77,30 @@ static int power_supply_hwmon_curr_to_property(u32 attr)
 	}
 }
 
+static int power_supply_hwmon_curr1_to_property(u32 attr)
+{
+	switch (attr) {
+	case hwmon_curr_max:
+		return POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT;
+	case hwmon_curr_input:
+		return POWER_SUPPLY_PROP_INPUT_CURRENT_NOW;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int power_supply_hwmon_curr_to_property(u32 attr, int channel)
+{
+	switch (channel) {
+	case 0:
+		return power_supply_hwmon_curr0_to_property(attr);
+	case 1:
+		return power_supply_hwmon_curr1_to_property(attr);
+	default:
+		return -EINVAL;
+	}
+}
+
 static int power_supply_hwmon_temp_to_property(u32 attr, int channel)
 {
 	if (channel) {
@@ -87,9 +140,9 @@ power_supply_hwmon_to_property(enum hwmon_sensor_types type,
 {
 	switch (type) {
 	case hwmon_in:
-		return power_supply_hwmon_in_to_property(attr);
+		return power_supply_hwmon_in_to_property(attr, channel);
 	case hwmon_curr:
-		return power_supply_hwmon_curr_to_property(attr);
+		return power_supply_hwmon_curr_to_property(attr, channel);
 	case hwmon_temp:
 		return power_supply_hwmon_temp_to_property(attr, channel);
 	default:
@@ -100,7 +153,9 @@ power_supply_hwmon_to_property(enum hwmon_sensor_types type,
 static bool power_supply_hwmon_is_a_label(enum hwmon_sensor_types type,
 					   u32 attr)
 {
-	return type == hwmon_temp && attr == hwmon_temp_label;
+	return (type == hwmon_temp && attr == hwmon_temp_label) ||
+	       (type == hwmon_in && attr == hwmon_in_label) ||
+	       (type == hwmon_curr && attr == hwmon_curr_label);
 }
 
 struct hwmon_type_attr_list {
@@ -114,7 +169,19 @@ static const u32 ps_temp_attrs[] = {
 	hwmon_temp_min_alarm, hwmon_temp_max_alarm,
 };
 
+static const u32 ps_in_attrs[] = {
+	hwmon_in_input, hwmon_in_average,
+	hwmon_in_min, hwmon_in_max,
+};
+
+static const u32 ps_curr_attrs[] = {
+	hwmon_curr_input, hwmon_curr_average,
+	hwmon_curr_max,
+};
+
 static const struct hwmon_type_attr_list ps_type_attrs[hwmon_max] = {
+	[hwmon_in] = { ps_in_attrs, ARRAY_SIZE(ps_in_attrs) },
+	[hwmon_curr] = { ps_curr_attrs, ARRAY_SIZE(ps_curr_attrs) },
 	[hwmon_temp] = { ps_temp_attrs, ARRAY_SIZE(ps_temp_attrs) },
 };
 
@@ -186,6 +253,11 @@ static int power_supply_hwmon_read_string(struct device *dev,
 					  const char **str)
 {
 	switch (type) {
+	case hwmon_in:
+	case hwmon_curr:
+		*str = ps_input_label[channel];
+		break;
+
 	case hwmon_temp:
 		*str = ps_temp_label[channel];
 		break;
@@ -309,15 +381,26 @@ static const struct hwmon_channel_info *power_supply_hwmon_info[] = {
 			   HWMON_T_MAX_ALARM),
 
 	HWMON_CHANNEL_INFO(curr,
+			   HWMON_C_LABEL   |
 			   HWMON_C_AVERAGE |
 			   HWMON_C_MAX     |
+			   HWMON_C_INPUT,
+
+			   HWMON_C_LABEL   |
+			   HWMON_C_MAX     |
 			   HWMON_C_INPUT),
 
 	HWMON_CHANNEL_INFO(in,
+			   HWMON_I_LABEL   |
 			   HWMON_I_AVERAGE |
 			   HWMON_I_MIN     |
 			   HWMON_I_MAX     |
+			   HWMON_I_INPUT,
+
+			   HWMON_I_LABEL   |
+			   HWMON_I_MAX     |
 			   HWMON_I_INPUT),
+
 	NULL
 };
 
@@ -382,6 +465,10 @@ int power_supply_add_hwmon_sysfs(struct power_supply *psy)
 		case POWER_SUPPLY_PROP_VOLTAGE_MIN:
 		case POWER_SUPPLY_PROP_VOLTAGE_MAX:
 		case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		case POWER_SUPPLY_PROP_INPUT_CURRENT_NOW:
+		case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		case POWER_SUPPLY_PROP_INPUT_VOLTAGE_NOW:
+		case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT:
 			set_bit(prop, psyhw->props);
 			break;
 		default:
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 51de3f47b25d..1d1fb69516a8 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -273,7 +273,9 @@ static struct device_attribute power_supply_attrs[] = {
 	POWER_SUPPLY_ATTR(charge_control_limit_max),
 	POWER_SUPPLY_ATTR(charge_control_start_threshold),
 	POWER_SUPPLY_ATTR(charge_control_end_threshold),
+	POWER_SUPPLY_ATTR(input_current_now),
 	POWER_SUPPLY_ATTR(input_current_limit),
+	POWER_SUPPLY_ATTR(input_voltage_now),
 	POWER_SUPPLY_ATTR(input_voltage_limit),
 	POWER_SUPPLY_ATTR(input_power_limit),
 	POWER_SUPPLY_ATTR(energy_full_design),
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 6a34df65d4d1..5313d1284aad 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -127,7 +127,9 @@ enum power_supply_property {
 	POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX,
 	POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD, /* in percents! */
 	POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD, /* in percents! */
+	POWER_SUPPLY_PROP_INPUT_CURRENT_NOW,
 	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
+	POWER_SUPPLY_PROP_INPUT_VOLTAGE_NOW,
 	POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT,
 	POWER_SUPPLY_PROP_INPUT_POWER_LIMIT,
 	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
-- 
2.20.1

[PATCH v4 4/4] power: supply: core: document measurement points

[Michał Mirosław]

Document used prefixes for input/output/storage voltages and currents.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 Documentation/power/power_supply_class.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/power/power_supply_class.rst b/Documentation/power/power_supply_class.rst
index 7b8c42f8b1de..c41b25aaa755 100644
--- a/Documentation/power/power_supply_class.rst
+++ b/Documentation/power/power_supply_class.rst
@@ -28,6 +28,12 @@ indication (including whether to use it at all) are fully controllable by
 user and/or specific machine defaults, per design principles of LED
 framework).
 
+There are three defined points of measurement for the benefit of mobile and
+UPS-like power supplies: INPUT (external power source), OUTPUT (power output
+from the module), and unmarked (power flowing to/from a storage element,
+eg. battery). Battery is viewed as a power source, so current flowing to
+the battery (charging it) is shown negative.
+
 
 Attributes/properties
 ~~~~~~~~~~~~~~~~~~~~~
-- 
2.20.1

[PATCH v4 3/4] power: supply: core: add output voltage measurements

[Michał Mirosław]

Add support for supply output voltage to be measured and configured.
This might be different from the voltage on the storage element (battery).

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/power/supply/power_supply_hwmon.c | 25 +++++++++++++++++++++++
 drivers/power/supply/power_supply_sysfs.c |  3 +++
 include/linux/power_supply.h              |  3 +++
 3 files changed, 31 insertions(+)

diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
index 3863b2a73ecf..626a344fbad1 100644
--- a/drivers/power/supply/power_supply_hwmon.c
+++ b/drivers/power/supply/power_supply_hwmon.c
@@ -16,6 +16,7 @@ struct power_supply_hwmon {
 static const char *const ps_input_label[] = {
 	"battery",
 	"external source",
+	"load",
 };
 
 static const char *const ps_temp_label[] = {
@@ -51,6 +52,20 @@ static int power_supply_hwmon_in1_to_property(u32 attr)
 	}
 }
 
+static int power_supply_hwmon_in2_to_property(u32 attr)
+{
+	switch (attr) {
+	case hwmon_in_min:
+		return POWER_SUPPLY_PROP_OUTPUT_VOLTAGE_MIN;
+	case hwmon_in_max:
+		return POWER_SUPPLY_PROP_OUTPUT_VOLTAGE_MAX;
+	case hwmon_in_input:
+		return POWER_SUPPLY_PROP_OUTPUT_VOLTAGE_NOW;
+	default:
+		return -EINVAL;
+	}
+}
+
 static int power_supply_hwmon_in_to_property(u32 attr, int channel)
 {
 	switch (channel) {
@@ -58,6 +73,8 @@ static int power_supply_hwmon_in_to_property(u32 attr, int channel)
 		return power_supply_hwmon_in0_to_property(attr);
 	case 1:
 		return power_supply_hwmon_in1_to_property(attr);
+	case 2:
+		return power_supply_hwmon_in2_to_property(attr);
 	default:
 		return -EINVAL;
 	}
@@ -399,6 +416,11 @@ static const struct hwmon_channel_info *power_supply_hwmon_info[] = {
 
 			   HWMON_I_LABEL   |
 			   HWMON_I_MAX     |
+			   HWMON_I_INPUT,
+
+			   HWMON_I_LABEL   |
+			   HWMON_I_MIN     |
+			   HWMON_I_MAX     |
 			   HWMON_I_INPUT),
 
 	NULL
@@ -469,6 +491,9 @@ int power_supply_add_hwmon_sysfs(struct power_supply *psy)
 		case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
 		case POWER_SUPPLY_PROP_INPUT_VOLTAGE_NOW:
 		case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT:
+		case POWER_SUPPLY_PROP_OUTPUT_VOLTAGE_MIN:
+		case POWER_SUPPLY_PROP_OUTPUT_VOLTAGE_MAX:
+		case POWER_SUPPLY_PROP_OUTPUT_VOLTAGE_NOW:
 			set_bit(prop, psyhw->props);
 			break;
 		default:
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 1d1fb69516a8..fb6f113b52bb 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -278,6 +278,9 @@ static struct device_attribute power_supply_attrs[] = {
 	POWER_SUPPLY_ATTR(input_voltage_now),
 	POWER_SUPPLY_ATTR(input_voltage_limit),
 	POWER_SUPPLY_ATTR(input_power_limit),
+	POWER_SUPPLY_ATTR(output_voltage_now),
+	POWER_SUPPLY_ATTR(output_voltage_min),
+	POWER_SUPPLY_ATTR(output_voltage_max),
 	POWER_SUPPLY_ATTR(energy_full_design),
 	POWER_SUPPLY_ATTR(energy_empty_design),
 	POWER_SUPPLY_ATTR(energy_full),
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 5313d1284aad..f1ff8d230488 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -132,6 +132,9 @@ enum power_supply_property {
 	POWER_SUPPLY_PROP_INPUT_VOLTAGE_NOW,
 	POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT,
 	POWER_SUPPLY_PROP_INPUT_POWER_LIMIT,
+	POWER_SUPPLY_PROP_OUTPUT_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_OUTPUT_VOLTAGE_MIN,
+	POWER_SUPPLY_PROP_OUTPUT_VOLTAGE_MAX,
 	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
 	POWER_SUPPLY_PROP_ENERGY_EMPTY_DESIGN,
 	POWER_SUPPLY_PROP_ENERGY_FULL,
-- 
2.20.1

Re: [PATCH v4 2/4] power: supply: core: add input voltage/current measurements

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3117 bytes --]

Hi,

On Fri, May 01, 2020 at 05:11:18PM +0200, Michał Mirosław wrote:
> Introduce input voltage and current limits and measurements.
> This makes room for e.g. VBUS measurements in USB chargers.

We already have properties for charger input voltage/current.
Unfortunately the naming is not as straight forward, as it
could be. Basically the properties have been added over time
and are ABI now. Things are documented in

Documentation/ABI/testing/sysfs-class-power

I provided the relevant properties below.

> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

[...]

> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -273,7 +273,9 @@ static struct device_attribute power_supply_attrs[] = {
>  	POWER_SUPPLY_ATTR(charge_control_limit_max),
>  	POWER_SUPPLY_ATTR(charge_control_start_threshold),
>  	POWER_SUPPLY_ATTR(charge_control_end_threshold),
> +	POWER_SUPPLY_ATTR(input_current_now),
>  	POWER_SUPPLY_ATTR(input_current_limit),
> +	POWER_SUPPLY_ATTR(input_voltage_now),
>  	POWER_SUPPLY_ATTR(input_voltage_limit),
>  	POWER_SUPPLY_ATTR(input_power_limit),
>  	POWER_SUPPLY_ATTR(energy_full_design),
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 6a34df65d4d1..5313d1284aad 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -127,7 +127,9 @@ enum power_supply_property {
>  	POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX,
>  	POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD, /* in percents! */
>  	POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD, /* in percents! */
> +	POWER_SUPPLY_PROP_INPUT_CURRENT_NOW,

What:           /sys/class/power_supply/<supply_name>/current_avg    
Date:           May 2007
Contact:        linux-pm@vger.kernel.org                          
Description:                  
                Reports an average IBUS current reading over a fixed period.   
                Normally devices will provide a fixed interval in which they   
                average readings to smooth out the reported value.             
                                                                                
                Access: Read    
                Valid values: Represented in microamps

>  	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
> +	POWER_SUPPLY_PROP_INPUT_VOLTAGE_NOW,

What:           /sys/class/power_supply/<supply_name>/voltage_now    
Date:           May 2007    
Contact:        linux-pm@vger.kernel.org    
Description:    
                Reports the VBUS voltage supplied now. This value is generally    
                read-only reporting, unless the 'online' state of the supply    
                is set to be programmable, in which case this value can be set    
                within the reported min/max range.    
    
                Access: Read, Write    
                Valid values: Represented in microvolts    

>  	POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT,
>  	POWER_SUPPLY_PROP_INPUT_POWER_LIMIT,
>  	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 1/4] power: supply: core: tabularize HWMON temperature labels

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1819 bytes --]

Hi,

On Fri, May 01, 2020 at 05:11:18PM +0200, Michał Mirosław wrote:
> Rework power_supply_hwmon_read_string() to check it's parameters.
> This allows to extend it later with labels for other types of
> measurements.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
> v2: split from fix temperature labels
> v3: remove power_supply_hwmon_read_string() parameter checks
>     as it is internal API (suggested by Guenter Roeck)
> v4: remove unreachable() as it triggers compiler bugs
> ---

Thanks, queued.

-- Sebastian

>  drivers/power/supply/power_supply_hwmon.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
> index af72e5693f65..f5d538485aaa 100644
> --- a/drivers/power/supply/power_supply_hwmon.c
> +++ b/drivers/power/supply/power_supply_hwmon.c
> @@ -13,6 +13,11 @@ struct power_supply_hwmon {
>  	unsigned long *props;
>  };
>  
> +static const char *const ps_temp_label[] = {
> +	"temp",
> +	"ambient temp",
> +};
> +
>  static int power_supply_hwmon_in_to_property(u32 attr)
>  {
>  	switch (attr) {
> @@ -180,7 +185,20 @@ static int power_supply_hwmon_read_string(struct device *dev,
>  					  u32 attr, int channel,
>  					  const char **str)
>  {
> -	*str = channel ? "temp ambient" : "temp";
> +	switch (type) {
> +	case hwmon_temp:
> +		*str = ps_temp_label[channel];
> +		break;
> +	default:
> +		/* unreachable, but see:
> +		 * gcc bug #51513 [1] and clang bug #978 [2]
> +		 *
> +		 * [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51513
> +		 * [2] https://github.com/ClangBuiltLinux/linux/issues/978
> +		 */
> +		break;
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.20.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 2/4] power: supply: core: add input voltage/current measurements

[Michał Mirosław]

On Sun, May 03, 2020 at 12:23:49AM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Fri, May 01, 2020 at 05:11:18PM +0200, Michał Mirosław wrote:
> > Introduce input voltage and current limits and measurements.
> > This makes room for e.g. VBUS measurements in USB chargers.
> We already have properties for charger input voltage/current.
> Unfortunately the naming is not as straight forward, as it
> could be. Basically the properties have been added over time
> and are ABI now. Things are documented in
> 
> Documentation/ABI/testing/sysfs-class-power
> 
> I provided the relevant properties below.

Hmm. Looks like there is no battery current/voltage properties then?
This is different from IBUS (input current), as IBUS = charging
current + system load. Documentation/power/power_supply_class.rst is
missing descriptions for the properties you mention.

[...]
> > --- a/include/linux/power_supply.h
> > +++ b/include/linux/power_supply.h
> > @@ -127,7 +127,9 @@ enum power_supply_property {
> >  	POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX,
> >  	POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD, /* in percents! */
> >  	POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD, /* in percents! */
> > +	POWER_SUPPLY_PROP_INPUT_CURRENT_NOW,
> 
> What:           /sys/class/power_supply/<supply_name>/current_avg    
> Date:           May 2007
> Contact:        linux-pm@vger.kernel.org                          
> Description:                  
>                 Reports an average IBUS current reading over a fixed period.   
>                 Normally devices will provide a fixed interval in which they   
>                 average readings to smooth out the reported value.             
>                                                                                 
>                 Access: Read    
>                 Valid values: Represented in microamps
> 

There are two entries for /sys/class/power_supply/<supply_name>/current_avg
in the file, the other one mentions IBAT instead. "voltage_now" has the
same problem. There seems to be a split-personality disorder present in
the kernel ABI. ;-)

Best Regards,
Michał Mirosław

Re: [PATCH v4 2/4] power: supply: core: add input voltage/current measurements

[Michał Mirosław]

On Sun, May 03, 2020 at 12:45:26AM +0200, Michał Mirosław wrote:
> On Sun, May 03, 2020 at 12:23:49AM +0200, Sebastian Reichel wrote:
> > On Fri, May 01, 2020 at 05:11:18PM +0200, Michał Mirosław wrote:
[...]
> > > --- a/include/linux/power_supply.h
> > > +++ b/include/linux/power_supply.h
> > > @@ -127,7 +127,9 @@ enum power_supply_property {
> > >  	POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX,
> > >  	POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD, /* in percents! */
> > >  	POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD, /* in percents! */
> > > +	POWER_SUPPLY_PROP_INPUT_CURRENT_NOW,
> > 
> > What:           /sys/class/power_supply/<supply_name>/current_avg    
> > Date:           May 2007
> > Contact:        linux-pm@vger.kernel.org                          
> > Description:                  
> >                 Reports an average IBUS current reading over a fixed period.   
> >                 Normally devices will provide a fixed interval in which they   
> >                 average readings to smooth out the reported value.             
> >                                                                                 
> >                 Access: Read    
> >                 Valid values: Represented in microamps
> > 
> 
> There are two entries for /sys/class/power_supply/<supply_name>/current_avg
> in the file, the other one mentions IBAT instead. "voltage_now" has the
> same problem.
[...]

So the general idea of the sysfs API seems to require separate devices for the
input (charger) and battery elements. Since what I'm looking at is an
integrated battery controller (bq25896) which has three connections: an USB
power (VBUS), a battery and the system load, but it creates only a single
power-class device. This is complicated by the fact that this is an OTG device
and so it can sink or source VBUS power.

Best Regards,
Michał Mirosław

Re: [PATCH v4 2/4] power: supply: core: add input voltage/current measurements

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2216 bytes --]

Hi,

On Sun, May 03, 2020 at 01:11:58AM +0200, Michał Mirosław wrote:
> On Sun, May 03, 2020 at 12:45:26AM +0200, Michał Mirosław wrote:
> > On Sun, May 03, 2020 at 12:23:49AM +0200, Sebastian Reichel wrote:
> > > On Fri, May 01, 2020 at 05:11:18PM +0200, Michał Mirosław wrote:
> [...]
> > > > --- a/include/linux/power_supply.h
> > > > +++ b/include/linux/power_supply.h
> > > > @@ -127,7 +127,9 @@ enum power_supply_property {
> > > >  	POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX,
> > > >  	POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD, /* in percents! */
> > > >  	POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD, /* in percents! */
> > > > +	POWER_SUPPLY_PROP_INPUT_CURRENT_NOW,
> > > 
> > > What:           /sys/class/power_supply/<supply_name>/current_avg    
> > > Date:           May 2007
> > > Contact:        linux-pm@vger.kernel.org                          
> > > Description:                  
> > >                 Reports an average IBUS current reading over a fixed period.   
> > >                 Normally devices will provide a fixed interval in which they   
> > >                 average readings to smooth out the reported value.             
> > >                                                                                 
> > >                 Access: Read    
> > >                 Valid values: Represented in microamps
> > > 
> > 
> > There are two entries for /sys/class/power_supply/<supply_name>/current_avg
> > in the file, the other one mentions IBAT instead. "voltage_now" has the
> > same problem.
> [...]
> 
> So the general idea of the sysfs API seems to require separate devices for the
> input (charger) and battery elements.
>
> Since what I'm looking at is an integrated battery controller
> (bq25896) which has three connections: an USB power (VBUS), a
> battery and the system load, but it creates only a single
> power-class device.

power-supply exposes either TYPE_MAINS/TYPE_USB or TYPE_BATTERY.
If a device is combined function, then it should register two
power-supply devices.

> This is complicated by the fact that this is an OTG device and so
> it can sink or source VBUS power.

Ok.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]