[GIT PULL] power_supply: add power supply scope

[GIT PULL] power_supply: add power supply scope

[Jeremy Fitzhardinge]

Hi,

This series adds a "scope" property to power supplies, so that a power
supply can indicate whether it powers the whole system, or just a
device.  This allows upowerd to distinguish between actual system power
supplies, and self-powered devices such as wireless mice.

Thanks,
    J

  Linux 3.2-rc2 (2011-11-15 15:02:59 -0200)

are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git power-supply-scope

Jeremy Fitzhardinge (3):
      power_supply: add SCOPE property to power supplies
      power_supply: allow powered device to be registered with supply
      power_supply: add scope properties to some common power_supplies

 drivers/acpi/ac.c                  |    4 ++++
 drivers/acpi/battery.c             |    5 +++++
 drivers/acpi/sbs.c                 |    9 +++++++++
 drivers/hid/hid-wacom.c            |   12 ++++++++++--
 drivers/hid/hid-wiimote.c          |    8 +++++++-
 drivers/power/power_supply_core.c  |    7 +++++++
 drivers/power/power_supply_sysfs.c |    6 ++++++
 include/linux/power_supply.h       |    8 ++++++++
 8 files changed, 56 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
index 6512b20..ec36c82 100644
--- a/drivers/acpi/ac.c
+++ b/drivers/acpi/ac.c
@@ -142,6 +142,9 @@ static int get_ac_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_ONLINE:
 		val->intval = ac->state;
 		break;
+	case POWER_SUPPLY_PROP_SCOPE:
+		val->intval = POWER_SUPPLY_SCOPE_SYSTEM;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -150,6 +153,7 @@ static int get_ac_property(struct power_supply *psy,
 
 static enum power_supply_property ac_props[] = {
 	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_SCOPE,
 };
 
 #ifdef CONFIG_ACPI_PROCFS_POWER
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 7711d94..1f68bb6 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -250,6 +250,9 @@ static int acpi_battery_get_property(struct power_supply *psy,
 		else
 			val->intval = battery->capacity_now * 1000;
 		break;
+	case POWER_SUPPLY_PROP_SCOPE:
+		val->intval = POWER_SUPPLY_SCOPE_SYSTEM;
+		break;
 	case POWER_SUPPLY_PROP_MODEL_NAME:
 		val->strval = battery->model_number;
 		break;
@@ -276,6 +279,7 @@ static enum power_supply_property charge_battery_props[] = {
 	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
 	POWER_SUPPLY_PROP_CHARGE_FULL,
 	POWER_SUPPLY_PROP_CHARGE_NOW,
+	POWER_SUPPLY_PROP_SCOPE,
 	POWER_SUPPLY_PROP_MODEL_NAME,
 	POWER_SUPPLY_PROP_MANUFACTURER,
 	POWER_SUPPLY_PROP_SERIAL_NUMBER,
@@ -292,6 +296,7 @@ static enum power_supply_property energy_battery_props[] = {
 	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
 	POWER_SUPPLY_PROP_ENERGY_FULL,
 	POWER_SUPPLY_PROP_ENERGY_NOW,
+	POWER_SUPPLY_PROP_SCOPE,
 	POWER_SUPPLY_PROP_MODEL_NAME,
 	POWER_SUPPLY_PROP_MANUFACTURER,
 	POWER_SUPPLY_PROP_SERIAL_NUMBER,
diff --git a/drivers/acpi/sbs.c b/drivers/acpi/sbs.c
index 6e36d0c..fc35e42 100644
--- a/drivers/acpi/sbs.c
+++ b/drivers/acpi/sbs.c
@@ -171,6 +171,9 @@ static int sbs_get_ac_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_ONLINE:
 		val->intval = sbs->charger_present;
 		break;
+	case POWER_SUPPLY_PROP_SCOPE:
+		val->intval = POWER_SUPPLY_SCOPE_SYSTEM;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -263,6 +266,9 @@ static int acpi_sbs_battery_get_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_TEMP:
 		val->intval = battery->temp_now - 2730;	// dK -> dC
 		break;
+	case POWER_SUPPLY_PROP_SCOPE:
+		val->intval = POWER_SUPPLY_SCOPE_SYSTEM;
+		break;
 	case POWER_SUPPLY_PROP_MODEL_NAME:
 		val->strval = battery->device_name;
 		break;
@@ -277,6 +283,7 @@ static int acpi_sbs_battery_get_property(struct power_supply *psy,
 
 static enum power_supply_property sbs_ac_props[] = {
 	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_SCOPE,
 };
 
 static enum power_supply_property sbs_charge_battery_props[] = {
@@ -292,6 +299,7 @@ static enum power_supply_property sbs_charge_battery_props[] = {
 	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
 	POWER_SUPPLY_PROP_CHARGE_FULL,
 	POWER_SUPPLY_PROP_CHARGE_NOW,
+	POWER_SUPPLY_PROP_SCOPE,
 	POWER_SUPPLY_PROP_TEMP,
 	POWER_SUPPLY_PROP_MODEL_NAME,
 	POWER_SUPPLY_PROP_MANUFACTURER,
@@ -311,6 +319,7 @@ static enum power_supply_property sbs_energy_battery_props[] = {
 	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
 	POWER_SUPPLY_PROP_ENERGY_FULL,
 	POWER_SUPPLY_PROP_ENERGY_NOW,
+	POWER_SUPPLY_PROP_SCOPE,
 	POWER_SUPPLY_PROP_TEMP,
 	POWER_SUPPLY_PROP_MODEL_NAME,
 	POWER_SUPPLY_PROP_MANUFACTURER,
diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c
index 17bb88f..ad39777 100644
--- a/drivers/hid/hid-wacom.c
+++ b/drivers/hid/hid-wacom.c
@@ -47,12 +47,14 @@ static unsigned short batcap[8] = { 1, 15, 25, 35, 50, 70, 100, 0 };
 
 static enum power_supply_property wacom_battery_props[] = {
 	POWER_SUPPLY_PROP_PRESENT,
-	POWER_SUPPLY_PROP_CAPACITY
+	POWER_SUPPLY_PROP_CAPACITY,
+	POWER_SUPPLY_PROP_SCOPE,
 };
 
 static enum power_supply_property wacom_ac_props[] = {
 	POWER_SUPPLY_PROP_PRESENT,
-	POWER_SUPPLY_PROP_ONLINE
+	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_SCOPE,
 };
 
 static int wacom_battery_get_property(struct power_supply *psy,
@@ -68,6 +70,9 @@ static int wacom_battery_get_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_PRESENT:
 		val->intval = 1;
 		break;
+	case POWER_SUPPLY_PROP_SCOPE:
+		val->intval = POWER_SUPPLY_SCOPE_DEVICE;
+		break;
 	case POWER_SUPPLY_PROP_CAPACITY:
 		/* show 100% battery capacity when charging */
 		if (power_state == 0)
@@ -99,6 +104,9 @@ static int wacom_ac_get_property(struct power_supply *psy,
 		else
 			val->intval = 0;
 		break;
+	case POWER_SUPPLY_PROP_SCOPE:
+		val->intval = POWER_SUPPLY_SCOPE_DEVICE;
+		break;
 	default:
 		ret = -EINVAL;
 		break;
diff --git a/drivers/hid/hid-wiimote.c b/drivers/hid/hid-wiimote.c
index 76739c0..98e4828 100644
--- a/drivers/hid/hid-wiimote.c
+++ b/drivers/hid/hid-wiimote.c
@@ -136,7 +136,8 @@ static __u16 wiiproto_keymap[] = {
 };
 
 static enum power_supply_property wiimote_battery_props[] = {
-	POWER_SUPPLY_PROP_CAPACITY
+	POWER_SUPPLY_PROP_CAPACITY,
+	POWER_SUPPLY_PROP_SCOPE,
 };
 
 /* requires the state.lock spinlock to be held */
@@ -468,6 +469,11 @@ static int wiimote_battery_get_property(struct power_supply *psy,
 	int ret = 0, state;
 	unsigned long flags;
 
+	if (psp == POWER_SUPPLY_PROP_SCOPE) {
+		val->intval = POWER_SUPPLY_SCOPE_DEVICE;
+		return 0;
+	}
+
 	ret = wiimote_cmd_acquire(wdata);
 	if (ret)
 		return ret;
diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index 329b46b..bacf327 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -147,6 +147,12 @@ struct power_supply *power_supply_get_by_name(char *name)
 }
 EXPORT_SYMBOL_GPL(power_supply_get_by_name);
 
+
+int power_supply_powers(struct power_supply *psy, struct device *dev)
+{
+	return sysfs_create_link_nowarn(&psy->dev->kobj, &dev->kobj, "powers");
+}
+
 static void power_supply_dev_release(struct device *dev)
 {
 	pr_debug("device: '%s': %s\n", dev_name(dev), __func__);
@@ -202,6 +208,7 @@ EXPORT_SYMBOL_GPL(power_supply_register);
 void power_supply_unregister(struct power_supply *psy)
 {
 	cancel_work_sync(&psy->changed_work);
+	sysfs_remove_link(&psy->dev->kobj, "powers");
 	power_supply_remove_triggers(psy);
 	device_unregister(psy->dev);
 }
diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
index e15d4c9..21178eb 100644
--- a/drivers/power/power_supply_sysfs.c
+++ b/drivers/power/power_supply_sysfs.c
@@ -63,6 +63,9 @@ static ssize_t power_supply_show_property(struct device *dev,
 	static char *capacity_level_text[] = {
 		"Unknown", "Critical", "Low", "Normal", "High", "Full"
 	};
+	static char *scope_text[] = {
+		"Unknown", "System", "Device"
+	};
 	ssize_t ret = 0;
 	struct power_supply *psy = dev_get_drvdata(dev);
 	const ptrdiff_t off = attr - power_supply_attrs;
@@ -95,6 +98,8 @@ static ssize_t power_supply_show_property(struct device *dev,
 		return sprintf(buf, "%s\n", capacity_level_text[value.intval]);
 	else if (off == POWER_SUPPLY_PROP_TYPE)
 		return sprintf(buf, "%s\n", type_text[value.intval]);
+	else if (off == POWER_SUPPLY_PROP_SCOPE)
+		return sprintf(buf, "%s\n", scope_text[value.intval]);
 	else if (off >= POWER_SUPPLY_PROP_MODEL_NAME)
 		return sprintf(buf, "%s\n", value.strval);
 
@@ -167,6 +172,7 @@ static struct device_attribute power_supply_attrs[] = {
 	POWER_SUPPLY_ATTR(time_to_full_now),
 	POWER_SUPPLY_ATTR(time_to_full_avg),
 	POWER_SUPPLY_ATTR(type),
+	POWER_SUPPLY_ATTR(scope),
 	/* Properties of type `const char *' */
 	POWER_SUPPLY_ATTR(model_name),
 	POWER_SUPPLY_ATTR(manufacturer),
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 204c18d..2e3c827 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -74,6 +74,12 @@ enum {
 	POWER_SUPPLY_CAPACITY_LEVEL_FULL,
 };
 
+enum {
+	POWER_SUPPLY_SCOPE_UNKNOWN = 0,
+	POWER_SUPPLY_SCOPE_SYSTEM,
+	POWER_SUPPLY_SCOPE_DEVICE,
+};
+
 enum power_supply_property {
 	/* Properties of type `int' */
 	POWER_SUPPLY_PROP_STATUS = 0,
@@ -116,6 +122,7 @@ enum power_supply_property {
 	POWER_SUPPLY_PROP_TIME_TO_FULL_NOW,
 	POWER_SUPPLY_PROP_TIME_TO_FULL_AVG,
 	POWER_SUPPLY_PROP_TYPE, /* use power_supply.type instead */
+	POWER_SUPPLY_PROP_SCOPE,
 	/* Properties of type `const char *' */
 	POWER_SUPPLY_PROP_MODEL_NAME,
 	POWER_SUPPLY_PROP_MANUFACTURER,
@@ -211,6 +218,7 @@ static inline int power_supply_is_system_supplied(void) { return -ENOSYS; }
 extern int power_supply_register(struct device *parent,
 				 struct power_supply *psy);
 extern void power_supply_unregister(struct power_supply *psy);
+extern int power_supply_powers(struct power_supply *psy, struct device *dev);
 
 /* For APM emulation, think legacy userspace. */
 extern struct class *power_supply_class;

Re: [GIT PULL] power_supply: add power supply scope

[Anton Vorontsov]

Hello Jeremy,

On Wed, Dec 07, 2011 at 05:41:37PM -0800, Jeremy Fitzhardinge wrote:
> This series adds a "scope" property to power supplies, so that a power
> supply can indicate whether it powers the whole system, or just a
> device.  This allows upowerd to distinguish between actual system power
> supplies, and self-powered devices such as wireless mice.

I see one problem with this approach. Userland will need patching
to distinguish system and device power supplies. So, even with
the patch applied, old userland will behave incorrectly.

So, instead of the new 'scope' property, how about adding another
power supply type? I.e. DEVICE_BATTERY ? That type would be unknown
to the current userland, and thus should be ignored.

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [GIT PULL] power_supply: add power supply scope

[Richard Hughes]

On 8 December 2011 10:02, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> I see one problem with this approach. Userland will need patching
> to distinguish system and device power supplies. So, even with
> the patch applied, old userland will behave incorrectly.

Userland will need patching anyway, as old versions of upower assumed
anything showing up in /sys/class/power_supply was powering the
system.

Richard.

Re: [GIT PULL] power_supply: add power supply scope

[Anton Vorontsov]

On Wed, Dec 07, 2011 at 05:41:37PM -0800, Jeremy Fitzhardinge wrote:
[...]
> diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
> index 6512b20..ec36c82 100644
> --- a/drivers/acpi/ac.c
> +++ b/drivers/acpi/ac.c
> @@ -142,6 +142,9 @@ static int get_ac_property(struct power_supply *psy,
>  	case POWER_SUPPLY_PROP_ONLINE:
>  		val->intval = ac->state;
>  		break;
> +	case POWER_SUPPLY_PROP_SCOPE:
> +		val->intval = POWER_SUPPLY_SCOPE_SYSTEM;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}

Mm... how about the rest of the drivers? I.e. drivers/power/*battery.c?

I think it's not a great idea to patch every driver, would be better to make
it similar to how we handle power_supply.type (w/ default value 'SYSTEM').


But, thinking more about it... personally, from the ABI point of view, I'd
like to just see some kind of 'supplicants' directory in the power_supply
instances, with symlinks to an appropriate devices.

I.e.
/sys/class/power_supply/battery/supplicants/<device_name>
is a symlink to /sys/class/HID/.../device.

With a special meaning of an empty directory (or non-existent, or w/ a
symlink pointing to '/sys/devices/system') -- system power.

That way we may describe any possible power hierarchy.

>From the implementation point of view, for now power_supply may just
conditionally (by introducing power_supply.not_system_power flag)
expose power_supply.dev->parent to /sys/.../supplicants.

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [GIT PULL] power_supply: add power supply scope

[Anton Vorontsov]

On Thu, Dec 08, 2011 at 10:05:46AM +0000, Richard Hughes wrote:
> On 8 December 2011 10:02, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> > I see one problem with this approach. Userland will need patching
> > to distinguish system and device power supplies. So, even with
> > the patch applied, old userland will behave incorrectly.
> 
> Userland will need patching anyway, as old versions of upower assumed
> anything showing up in /sys/class/power_supply was powering the
> system.

Oh, makes sense then. But see my other email regarding a bit more
universal scheme.

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [GIT PULL] power_supply: add power supply scope

[Jeremy Fitzhardinge]

On 12/08/2011 02:41 AM, Anton Vorontsov wrote:
> On Wed, Dec 07, 2011 at 05:41:37PM -0800, Jeremy Fitzhardinge wrote:
> [...]
>> diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
>> index 6512b20..ec36c82 100644
>> --- a/drivers/acpi/ac.c
>> +++ b/drivers/acpi/ac.c
>> @@ -142,6 +142,9 @@ static int get_ac_property(struct power_supply *psy,
>>  	case POWER_SUPPLY_PROP_ONLINE:
>>  		val->intval = ac->state;
>>  		break;
>> +	case POWER_SUPPLY_PROP_SCOPE:
>> +		val->intval = POWER_SUPPLY_SCOPE_SYSTEM;
>> +		break;
>>  	default:
>>  		return -EINVAL;
>>  	}
> Mm... how about the rest of the drivers? I.e. drivers/power/*battery.c?
>
> I think it's not a great idea to patch every driver, would be better to make
> it similar to how we handle power_supply.type (w/ default value 'SYSTEM').

Yes.  That patch was mostly so I could test the mechanism.  Certainly
general rule is that if there's no scope attribute then assume System.

> But, thinking more about it... personally, from the ABI point of view, I'd
> like to just see some kind of 'supplicants' directory in the power_supply
> instances, with symlinks to an appropriate devices.

Yes, that was my first proposal, and I have a patch to allow Device
scope power supplies to indicate which "device" it is (which may be a
root of a subtree of devices).  I'm not too sure about making it
multiple devices; at that point I'd be tempted to introduce the notion
of a "power bus" which points to multiple devices and make power
supplies point to that.

> I.e.
> /sys/class/power_supply/battery/supplicants/<device_name>
> is a symlink to /sys/class/HID/.../device.
>
> With a special meaning of an empty directory (or non-existent, or w/ a
> symlink pointing to '/sys/devices/system') -- system power.

Yes.  That's awkward to implement because the kobj isn't exported from
device/base.  But aside from that, its a somewhat awkward interface for
usermode, because it has to end up following symlink and resolving their
paths, and then having special hardcoded knowledge of what particular
paths mean.  When all upower really wants to know is "do I need to
suspend when this supply gets low?".

> That way we may describe any possible power hierarchy.
>
> From the implementation point of view, for now power_supply may just
> conditionally (by introducing power_supply.not_system_power flag)

How is that different from scope?

    J

Re: [GIT PULL] power_supply: add power supply scope

[Anton Vorontsov]

On Thu, Dec 08, 2011 at 08:53:15AM -0800, Jeremy Fitzhardinge wrote:
> Yes.  That patch was mostly so I could test the mechanism.  Certainly
> general rule is that if there's no scope attribute then assume System.

Okay, great.

> > /sys/class/power_supply/battery/supplicants/<device_name>
> > is a symlink to /sys/class/HID/.../device.
> >
> > With a special meaning of an empty directory (or non-existent, or w/ a
> > symlink pointing to '/sys/devices/system') -- system power.
> 
> Yes.  That's awkward to implement because the kobj isn't exported from
> device/base.  But aside from that, its a somewhat awkward interface for
> usermode, because it has to end up following symlink and resolving their
> paths, and then having special hardcoded knowledge of what particular
> paths mean.  When all upower really wants to know is "do I need to
> suspend when this supply gets low?".

Mm... OK. I think you're right. The 'scope' thing is indeed useful by
itself.

> > That way we may describe any possible power hierarchy.
> >
> > From the implementation point of view, for now power_supply may just
> > conditionally (by introducing power_supply.not_system_power flag)
> 
> How is that different from scope?

No different at all, I'm fine with either power_supply.scope or any
other flag. :-)

Thanks!

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [GIT PULL] power_supply: add power supply scope

[Jeremy Fitzhardinge]

On 12/08/2011 03:36 PM, Anton Vorontsov wrote:
>
> No different at all, I'm fine with either power_supply.scope or any
> other flag. :-)

So are you happy to pull that branch, or did you have some other
concerns with it?

Thanks,
    J

Re: [GIT PULL] power_supply: add power supply scope

[Anton Vorontsov]

On Fri, Dec 09, 2011 at 12:18:47AM -0800, Jeremy Fitzhardinge wrote:
> On 12/08/2011 03:36 PM, Anton Vorontsov wrote:
> >
> > No different at all, I'm fine with either power_supply.scope or any
> > other flag. :-)
> 
> So are you happy to pull that branch, or did you have some other
> concerns with it?

I'm OK w/ the idea itself, but to actually pull that branch, the issue
w/ needless patching of battery drivers has to be fixed (i.e. drop the
changes in the drivers/acpi/).

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [GIT PULL] power_supply: add power supply scope

[Anton Vorontsov]

A few more minor nits...

On Wed, Dec 07, 2011 at 05:41:37PM -0800, Jeremy Fitzhardinge wrote:
[...]
> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
> index 329b46b..bacf327 100644
> --- a/drivers/power/power_supply_core.c
> +++ b/drivers/power/power_supply_core.c
> @@ -147,6 +147,12 @@ struct power_supply *power_supply_get_by_name(char *name)
>  }
>  EXPORT_SYMBOL_GPL(power_supply_get_by_name);
>  
> +

Needless newline. :-)

> +int power_supply_powers(struct power_supply *psy, struct device *dev)
> +{
> +	return sysfs_create_link_nowarn(&psy->dev->kobj, &dev->kobj, "powers");
> +}

- This surely creates an important ABI to the userland. This has to be
  documented via Documentation/ABI (or at least via commit message that
  I don't see in this email :-).

- This functionality isn't used anywhere as of now (i.e. I don't see
  anyone calling this function), so let's omit it for now?

- I still don't think that this should be a single symlink, to me the
  more universal (so that we don't have to break ABI in the future)
  way would be 'powers' directory with symlinks in it.
  But maybe I'm not following where exactly the link will be created?
  Documentation or an example of the new sysfs structure would help.

> +
>  static void power_supply_dev_release(struct device *dev)
>  {
>  	pr_debug("device: '%s': %s\n", dev_name(dev), __func__);
> @@ -202,6 +208,7 @@ EXPORT_SYMBOL_GPL(power_supply_register);
>  void power_supply_unregister(struct power_supply *psy)
>  {
>  	cancel_work_sync(&psy->changed_work);
> +	sysfs_remove_link(&psy->dev->kobj, "powers");
>  	power_supply_remove_triggers(psy);
>  	device_unregister(psy->dev);
>  }
> diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
> index e15d4c9..21178eb 100644
> --- a/drivers/power/power_supply_sysfs.c
> +++ b/drivers/power/power_supply_sysfs.c
> @@ -63,6 +63,9 @@ static ssize_t power_supply_show_property(struct device *dev,
>  	static char *capacity_level_text[] = {
>  		"Unknown", "Critical", "Low", "Normal", "High", "Full"
>  	};
> +	static char *scope_text[] = {
> +		"Unknown", "System", "Device"
> +	};
>  	ssize_t ret = 0;
>  	struct power_supply *psy = dev_get_drvdata(dev);
>  	const ptrdiff_t off = attr - power_supply_attrs;
> @@ -95,6 +98,8 @@ static ssize_t power_supply_show_property(struct device *dev,
>  		return sprintf(buf, "%s\n", capacity_level_text[value.intval]);
>  	else if (off == POWER_SUPPLY_PROP_TYPE)
>  		return sprintf(buf, "%s\n", type_text[value.intval]);
> +	else if (off == POWER_SUPPLY_PROP_SCOPE)
> +		return sprintf(buf, "%s\n", scope_text[value.intval]);

Should we really handle the PROP_SCOPE as a dynamic property?
Maybe do it similar to PROP_TYPE, so that drivers will only need to
specity the scope during registration, and not bother w/ handling
it in theirs get_property() callbacks?

Thanks!

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [GIT PULL] power_supply: add power supply scope

[Jeremy Fitzhardinge]

On 12/09/2011 01:59 AM, Anton Vorontsov wrote:
> On Fri, Dec 09, 2011 at 12:18:47AM -0800, Jeremy Fitzhardinge wrote:
>> On 12/08/2011 03:36 PM, Anton Vorontsov wrote:
>>> No different at all, I'm fine with either power_supply.scope or any
>>> other flag. :-)
>> So are you happy to pull that branch, or did you have some other
>> concerns with it?
> I'm OK w/ the idea itself, but to actually pull that branch, the issue
> w/ needless patching of battery drivers has to be fixed (i.e. drop the
> changes in the drivers/acpi/).

OK, and just use the default System?  So only patch devices which are
actually self-powered?

    J

Re: [GIT PULL] power_supply: add power supply scope

[Jeremy Fitzhardinge]

On 12/09/2011 02:17 AM, Anton Vorontsov wrote:
> A few more minor nits...
>
> On Wed, Dec 07, 2011 at 05:41:37PM -0800, Jeremy Fitzhardinge wrote:
> [...]
>> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
>> index 329b46b..bacf327 100644
>> --- a/drivers/power/power_supply_core.c
>> +++ b/drivers/power/power_supply_core.c
>> @@ -147,6 +147,12 @@ struct power_supply *power_supply_get_by_name(char *name)
>>  }
>>  EXPORT_SYMBOL_GPL(power_supply_get_by_name);
>>  
>> +
> Needless newline. :-)

OK.

>> +int power_supply_powers(struct power_supply *psy, struct device *dev)
>> +{
>> +	return sysfs_create_link_nowarn(&psy->dev->kobj, &dev->kobj, "powers");
>> +}
> - This surely creates an important ABI to the userland. This has to be
>   documented via Documentation/ABI (or at least via commit message that
>   I don't see in this email :-).

Sure, I'll add that.  It doesn't look like Documentation/ABI has
anything about the whole power_supply class, so I'll consider adding
that as out of scope for this work, but I'll fill out the commit comment.

> - This functionality isn't used anywhere as of now (i.e. I don't see
>   anyone calling this function), so let's omit it for now?

I have a patch in my hid-input battery series which use it.  Actually, I
can use it in this series for the Wacom and Wiimote HID power supplies.

> - I still don't think that this should be a single symlink, to me the
>   more universal (so that we don't have to break ABI in the future)
>   way would be 'powers' directory with symlinks in it.
>   But maybe I'm not following where exactly the link will be created?
>   Documentation or an example of the new sysfs structure would help.

At the moment, it's a pointer to a single device, which may have
sub-devices under it.  This matches the general device tree power
management model which assumes that the bus topology of the system is
also a reasonable approximation for the power distribution topology (if
nothing else, because if you power off a bridge device, you can't see
anything behind it so it may as well be considered powered off as well).

I understand your point about powering multiple devices, but it seems
like overengineering to implement it now unless you have some specific
users of that interface in mind.  If you did want to support a single
power supply powering multiple devices in future, you could add the
notion of a "power bus" device which distributes power to multiple
devices, without having to complicate the common case of powering a
single device, and without having to change the current semantics of the
"powers" link.

>> diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
>> index e15d4c9..21178eb 100644
>> --- a/drivers/power/power_supply_sysfs.c
>> +++ b/drivers/power/power_supply_sysfs.c
>> @@ -63,6 +63,9 @@ static ssize_t power_supply_show_property(struct device *dev,
>>  	static char *capacity_level_text[] = {
>>  		"Unknown", "Critical", "Low", "Normal", "High", "Full"
>>  	};
>> +	static char *scope_text[] = {
>> +		"Unknown", "System", "Device"
>> +	};
>>  	ssize_t ret = 0;
>>  	struct power_supply *psy = dev_get_drvdata(dev);
>>  	const ptrdiff_t off = attr - power_supply_attrs;
>> @@ -95,6 +98,8 @@ static ssize_t power_supply_show_property(struct device *dev,
>>  		return sprintf(buf, "%s\n", capacity_level_text[value.intval]);
>>  	else if (off == POWER_SUPPLY_PROP_TYPE)
>>  		return sprintf(buf, "%s\n", type_text[value.intval]);
>> +	else if (off == POWER_SUPPLY_PROP_SCOPE)
>> +		return sprintf(buf, "%s\n", scope_text[value.intval]);
> Should we really handle the PROP_SCOPE as a dynamic property?
> Maybe do it similar to PROP_TYPE, so that drivers will only need to
> specity the scope during registration, and not bother w/ handling
> it in theirs get_property() callbacks?

I don't really know.  I guess its possible in theory that a device could
change scope on the fly if its power was re-routed.  But then, the type
can change too (if, for example, a UPS switched between AC and battery
power, or a HID device switching between corded USB power or cordless
battery power), so I'm not really sure what the rationale is either
way.  (I guess you model power supplies switching type as having
multiple power supplies which turn themselves on and offline?)

    J

Re: [GIT PULL] power_supply: add power supply scope

[Daniel Nicoletti]

>>> diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
>>> index e15d4c9..21178eb 100644
>>> --- a/drivers/power/power_supply_sysfs.c
>>> +++ b/drivers/power/power_supply_sysfs.c
>>> @@ -63,6 +63,9 @@ static ssize_t power_supply_show_property(struct device *dev,
>>>      static char *capacity_level_text[] = {
>>>              "Unknown", "Critical", "Low", "Normal", "High", "Full"
>>>      };
>>> +    static char *scope_text[] = {
>>> +            "Unknown", "System", "Device"
>>> +    };
>>>      ssize_t ret = 0;
>>>      struct power_supply *psy = dev_get_drvdata(dev);
>>>      const ptrdiff_t off = attr - power_supply_attrs;
>>> @@ -95,6 +98,8 @@ static ssize_t power_supply_show_property(struct device *dev,
>>>              return sprintf(buf, "%s\n", capacity_level_text[value.intval]);
>>>      else if (off == POWER_SUPPLY_PROP_TYPE)
>>>              return sprintf(buf, "%s\n", type_text[value.intval]);
>>> +    else if (off == POWER_SUPPLY_PROP_SCOPE)
>>> +            return sprintf(buf, "%s\n", scope_text[value.intval]);
>> Should we really handle the PROP_SCOPE as a dynamic property?
>> Maybe do it similar to PROP_TYPE, so that drivers will only need to
>> specity the scope during registration, and not bother w/ handling
>> it in theirs get_property() callbacks?
>
> I don't really know.  I guess its possible in theory that a device could
> change scope on the fly if its power was re-routed.  But then, the type
> can change too (if, for example, a UPS switched between AC and battery
> power, or a HID device switching between corded USB power or cordless
> battery power), so I'm not really sure what the rationale is either
> way.  (I guess you model power supplies switching type as having
> multiple power supplies which turn themselves on and offline?)

But isn't the scope about powering or not the system? If so even if
the device is now using AC it will not be powering the computer.

Re: [GIT PULL] power_supply: add power supply scope

[Jeremy Fitzhardinge]

On 12/09/2011 12:00 PM, Daniel Nicoletti wrote:
>>>> diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
>>>> index e15d4c9..21178eb 100644
>>>> --- a/drivers/power/power_supply_sysfs.c
>>>> +++ b/drivers/power/power_supply_sysfs.c
>>>> @@ -63,6 +63,9 @@ static ssize_t power_supply_show_property(struct device *dev,
>>>>      static char *capacity_level_text[] = {
>>>>              "Unknown", "Critical", "Low", "Normal", "High", "Full"
>>>>      };
>>>> +    static char *scope_text[] = {
>>>> +            "Unknown", "System", "Device"
>>>> +    };
>>>>      ssize_t ret = 0;
>>>>      struct power_supply *psy = dev_get_drvdata(dev);
>>>>      const ptrdiff_t off = attr - power_supply_attrs;
>>>> @@ -95,6 +98,8 @@ static ssize_t power_supply_show_property(struct device *dev,
>>>>              return sprintf(buf, "%s\n", capacity_level_text[value.intval]);
>>>>      else if (off == POWER_SUPPLY_PROP_TYPE)
>>>>              return sprintf(buf, "%s\n", type_text[value.intval]);
>>>> +    else if (off == POWER_SUPPLY_PROP_SCOPE)
>>>> +            return sprintf(buf, "%s\n", scope_text[value.intval]);
>>> Should we really handle the PROP_SCOPE as a dynamic property?
>>> Maybe do it similar to PROP_TYPE, so that drivers will only need to
>>> specity the scope during registration, and not bother w/ handling
>>> it in theirs get_property() callbacks?
>> I don't really know.  I guess its possible in theory that a device could
>> change scope on the fly if its power was re-routed.  But then, the type
>> can change too (if, for example, a UPS switched between AC and battery
>> power, or a HID device switching between corded USB power or cordless
>> battery power), so I'm not really sure what the rationale is either
>> way.  (I guess you model power supplies switching type as having
>> multiple power supplies which turn themselves on and offline?)
> But isn't the scope about powering or not the system? If so even if
> the device is now using AC it will not be powering the computer.
>

Sure.  I was just commenting on the similarity between scope and type
with respect to whether they're immutable properties of a power supply
or things that can change over time.

    J