Introduce a mechanism for drivers to extend the properties implemented
by a power supply.
Motivation
----------
Various drivers, mostly in platform/x86 extend the ACPI battery driver
with additional sysfs attributes to implement more UAPIs than are
exposed through ACPI by using various side-channels, like WMI,
nonstandard ACPI or EC communication.
While the created sysfs attributes look similar to the attributes
provided by the powersupply core, there are various deficiencies:
* They don't show up in uevent payload.
* They can't be queried with the standard in-kernel APIs.
* They don't work with triggers.
* The extending driver has to reimplement all of the parsing,
formatting and sysfs display logic.
* Writing a extension driver is completely different from writing a
normal power supply driver.
* ~Properties can not be properly overriden.~
(Overriding is not implemented anymore, can be readded)
The proposed extension API avoids all of these issues.
An extension is just a "struct power_supply_ext" with the same kind of
callbacks as in a normal "struct power_supply_desc".
The API is meant to be used via battery_hook_register(), the same way as
the current extensions.
For example my upcoming cros_ec charge control driver[0] saves 80 lines
of code with this patchset.
Contents
--------
* Patch 1 and 2 are generic preparation patches, that probably make
sense without this series.
* Patch 3 implements the extension API itself.
* Patch 4 implements a PoC locking scheme for the extension API.
* Patch 5 adds extension support to test_power.c
* Patch 6 converts the in-tree platform/x86/system76 driver to the
extension API.
Open issues
-----------
* Newly registered properties will not show up in hwmon.
To do that properly would require some changes in the hwmon core.
As far as I know, no current driver would extend the hwmon properties anyways.
* As this is only useful with the hooks of CONFIG_ACPI_BATTERY, should
it also be gated behind this or another config?
* Only one extension can be used at a time.
So far this should be enough, having more would complicate the
implementation.
* Is an rw_semaphore acceptable?
[0] https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Thomas Weißschuh <[email protected]>
---
Changes in v2:
- Drop locking patch, let's figure out the API first
- Allow registration of multiple extensions
- Pass extension to extension callbacks as parameter
- Disallow property overlap between extensions and core psy
- Drop system76/pdx86 maintainers, as the system76 changes are only RFC
state anyways
- Link to v1: https://lore.kernel.org/r/20240606-power-supply-extensions-v1-0-b45669290bdc@weissschuh.net
---
Thomas Weißschuh (5):
power: supply: sysfs: use power_supply_property_is_writeable()
power: supply: core: avoid iterating properties directly
power: supply: core: implement extension API
power: supply: test-power: implement a power supply extension
platform/x86: system76: Use power_supply extension API
drivers/platform/x86/system76_acpi.c | 93 ++++++++++++----------
drivers/power/supply/power_supply.h | 12 +++
drivers/power/supply/power_supply_core.c | 128 ++++++++++++++++++++++++++++--
drivers/power/supply/power_supply_hwmon.c | 48 +++++------
drivers/power/supply/power_supply_sysfs.c | 50 +++++++++---
drivers/power/supply/test_power.c | 101 +++++++++++++++++++++++
include/linux/power_supply.h | 26 ++++++
7 files changed, 371 insertions(+), 87 deletions(-)
---
base-commit: 061d1af7b0305227182bd9da60c7706c079348b7
change-id: 20240602-power-supply-extensions-07d949f509d9
Best regards,
--
Thomas Weißschuh <[email protected]>
Signed-off-by: Thomas Weißschuh <[email protected]>
---
drivers/power/supply/power_supply.h | 13 ++-
drivers/power/supply/power_supply_core.c | 128 ++++++++++++++++++++++++++++--
drivers/power/supply/power_supply_hwmon.c | 2 +-
drivers/power/supply/power_supply_sysfs.c | 37 ++++++++-
include/linux/power_supply.h | 26 ++++++
5 files changed, 192 insertions(+), 14 deletions(-)
diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
index 622be1f0a180..686b66161900 100644
--- a/drivers/power/supply/power_supply.h
+++ b/drivers/power/supply/power_supply.h
@@ -13,8 +13,17 @@ struct device;
struct device_type;
struct power_supply;
-extern bool power_supply_has_property(const struct power_supply_desc *psy_desc,
- enum power_supply_property psp);
+struct psy_ext_registration {
+ struct list_head list_head;
+ const struct power_supply_ext *ext;
+};
+
+#define psy_for_each_extension(psy, pos) list_for_each_entry(pos, &(psy)->extensions, list_head)
+
+bool power_supply_has_property(const struct power_supply *psy,
+ enum power_supply_property psp);
+bool power_supply_ext_has_property(const struct power_supply_ext *psy_ext,
+ enum power_supply_property psp);
#ifdef CONFIG_SYSFS
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index d57ecdd966e0..fcdfedf3df7c 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -1183,8 +1183,8 @@ bool power_supply_battery_bti_in_range(struct power_supply_battery_info *info,
}
EXPORT_SYMBOL_GPL(power_supply_battery_bti_in_range);
-bool power_supply_has_property(const struct power_supply_desc *psy_desc,
- enum power_supply_property psp)
+static bool psy_desc_has_property(const struct power_supply_desc *psy_desc,
+ enum power_supply_property psp)
{
bool found = false;
int i;
@@ -1199,17 +1199,55 @@ bool power_supply_has_property(const struct power_supply_desc *psy_desc,
return found;
}
+bool power_supply_ext_has_property(const struct power_supply_ext *psy_ext,
+ enum power_supply_property psp)
+{
+ bool found = false;
+ int i;
+
+ if (!psy_ext)
+ return false;
+
+ for (i = 0; i < psy_ext->num_properties; i++) {
+ if (psy_ext->properties[i] == psp) {
+ found = true;
+ break;
+ }
+ }
+
+ return found;
+}
+
+bool power_supply_has_property(const struct power_supply *psy,
+ enum power_supply_property psp)
+{
+ struct psy_ext_registration *reg;
+
+ psy_for_each_extension(psy, reg)
+ if (power_supply_ext_has_property(reg->ext, psp))
+ return true;
+
+ return psy_desc_has_property(psy->desc, psp);
+}
+
int power_supply_get_property(struct power_supply *psy,
enum power_supply_property psp,
union power_supply_propval *val)
{
+ struct psy_ext_registration *reg;
+
if (atomic_read(&psy->use_cnt) <= 0) {
if (!psy->initialized)
return -EAGAIN;
return -ENODEV;
}
- if (power_supply_has_property(psy->desc, psp))
+ psy_for_each_extension(psy, reg) {
+ if (power_supply_ext_has_property(reg->ext, psp))
+ return reg->ext->get_property(psy, reg->ext, psp, val);
+ }
+
+ if (psy_desc_has_property(psy->desc, psp))
return psy->desc->get_property(psy, psp, val);
else if (power_supply_battery_info_has_prop(psy->battery_info, psp))
return power_supply_battery_info_get_prop(psy->battery_info, psp, val);
@@ -1222,7 +1260,21 @@ int power_supply_set_property(struct power_supply *psy,
enum power_supply_property psp,
const union power_supply_propval *val)
{
- if (atomic_read(&psy->use_cnt) <= 0 || !psy->desc->set_property)
+ struct psy_ext_registration *reg;
+
+ if (atomic_read(&psy->use_cnt) <= 0)
+ return -ENODEV;
+
+ psy_for_each_extension(psy, reg) {
+ if (power_supply_ext_has_property(reg->ext, psp)) {
+ if (reg->ext->set_property)
+ return reg->ext->set_property(psy, reg->ext, psp, val);
+ else
+ return -ENODEV;
+ }
+ }
+
+ if (!psy->desc->set_property)
return -ENODEV;
return psy->desc->set_property(psy, psp, val);
@@ -1232,8 +1284,21 @@ EXPORT_SYMBOL_GPL(power_supply_set_property);
int power_supply_property_is_writeable(struct power_supply *psy,
enum power_supply_property psp)
{
- if (atomic_read(&psy->use_cnt) <= 0 ||
- !psy->desc->property_is_writeable)
+ struct psy_ext_registration *reg;
+
+ if (atomic_read(&psy->use_cnt) <= 0)
+ return -ENODEV;
+
+ psy_for_each_extension(psy, reg) {
+ if (power_supply_ext_has_property(reg->ext, psp)) {
+ if (reg->ext->property_is_writeable)
+ return reg->ext->property_is_writeable(psy, reg->ext, psp);
+ else
+ return -ENODEV;
+ }
+ }
+
+ if (!psy->desc->property_is_writeable)
return -ENODEV;
return psy->desc->property_is_writeable(psy, psp);
@@ -1256,6 +1321,52 @@ int power_supply_powers(struct power_supply *psy, struct device *dev)
}
EXPORT_SYMBOL_GPL(power_supply_powers);
+static int power_supply_update_groups(struct power_supply *psy)
+{
+ int ret;
+
+ ret = sysfs_update_groups(&psy->dev.kobj, power_supply_dev_type.groups);
+ power_supply_changed(psy);
+ return ret;
+}
+
+int power_supply_register_extension(struct power_supply *psy, const struct power_supply_ext *ext)
+{
+ struct psy_ext_registration *reg;
+ size_t i;
+
+ for (i = 0; i < ext->num_properties; i++) {
+ if (power_supply_has_property(psy, ext->properties[i]))
+ return -EEXIST;
+ }
+
+ reg = devm_kmalloc(&psy->dev, sizeof(*reg), GFP_KERNEL);
+ if (!reg)
+ return -ENOMEM;
+
+ reg->ext = ext;
+ list_add(®->list_head, &psy->extensions);
+
+ return power_supply_update_groups(psy);
+}
+EXPORT_SYMBOL_GPL(power_supply_register_extension);
+
+void power_supply_unregister_extension(struct power_supply *psy, const struct power_supply_ext *ext)
+{
+ struct psy_ext_registration *reg;
+
+ psy_for_each_extension(psy, reg) {
+ if (reg->ext == ext) {
+ list_del(®->list_head);
+ power_supply_update_groups(psy);
+ return;
+ }
+ }
+
+ dev_warn(&psy->dev, "Trying to unregister invalid extension");
+}
+EXPORT_SYMBOL_GPL(power_supply_unregister_extension);
+
static void power_supply_dev_release(struct device *dev)
{
struct power_supply *psy = to_power_supply(dev);
@@ -1308,7 +1419,7 @@ static int psy_register_thermal(struct power_supply *psy)
return 0;
/* Register battery zone device psy reports temperature */
- if (power_supply_has_property(psy->desc, POWER_SUPPLY_PROP_TEMP)) {
+ if (power_supply_has_property(psy, POWER_SUPPLY_PROP_TEMP)) {
/* Prefer our hwmon device and avoid duplicates */
struct thermal_zone_params tzp = {
.no_hwmon = IS_ENABLED(CONFIG_POWER_SUPPLY_HWMON)
@@ -1361,7 +1472,7 @@ __power_supply_register(struct device *parent,
pr_warn("%s: Expected proper parent device for '%s'\n",
__func__, desc->name);
- if (power_supply_has_property(desc, POWER_SUPPLY_PROP_USB_TYPE) &&
+ if (psy_desc_has_property(desc, POWER_SUPPLY_PROP_USB_TYPE) &&
(!desc->usb_types || !desc->num_usb_types))
return ERR_PTR(-EINVAL);
@@ -1415,6 +1526,7 @@ __power_supply_register(struct device *parent,
}
spin_lock_init(&psy->changed_lock);
+ INIT_LIST_HEAD(&psy->extensions);
rc = device_add(dev);
if (rc)
goto device_add_failed;
diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
index 2ecbe4a74c25..8cb852a734b1 100644
--- a/drivers/power/supply/power_supply_hwmon.c
+++ b/drivers/power/supply/power_supply_hwmon.c
@@ -374,7 +374,7 @@ int power_supply_add_hwmon_sysfs(struct power_supply *psy)
for (i = 0; i < ARRAY_SIZE(power_supply_hwmon_props); i++) {
const enum power_supply_property prop = power_supply_hwmon_props[i];
- if (power_supply_has_property(psy->desc, prop))
+ if (power_supply_has_property(psy, prop))
set_bit(prop, psyhw->props);
}
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index abd44ebfe6fe..8f29ddea08d0 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -271,6 +271,23 @@ static ssize_t power_supply_show_usb_type(struct device *dev,
return count;
}
+static ssize_t power_supply_show_charge_behaviour(struct device *dev,
+ struct power_supply *psy,
+ union power_supply_propval *value,
+ char *buf)
+{
+ struct psy_ext_registration *reg;
+
+ psy_for_each_extension(psy, reg) {
+ if (power_supply_ext_has_property(reg->ext, POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR))
+ return power_supply_charge_behaviour_show(dev, reg->ext->charge_behaviours,
+ value->intval, buf);
+ }
+
+ return power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours,
+ value->intval, buf);
+}
+
static ssize_t power_supply_show_property(struct device *dev,
struct device_attribute *attr,
char *buf) {
@@ -304,8 +321,7 @@ static ssize_t power_supply_show_property(struct device *dev,
&value, buf);
break;
case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
- ret = power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours,
- value.intval, buf);
+ ret = power_supply_show_charge_behaviour(dev, psy, &value, buf);
break;
case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER:
ret = sysfs_emit(buf, "%s\n", value.strval);
@@ -374,7 +390,7 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
if (attrno == POWER_SUPPLY_PROP_TYPE)
return mode;
- if (power_supply_has_property(psy->desc, attrno)) {
+ if (power_supply_has_property(psy, attrno)) {
if (power_supply_property_is_writeable(psy, attrno) > 0)
mode |= S_IWUSR;
return mode;
@@ -459,6 +475,7 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
power_supply_battery_info_properties;
unsigned long psy_drv_properties[POWER_SUPPLY_ATTR_CNT /
sizeof(unsigned long) + 1] = {0};
+ struct psy_ext_registration *ext;
int ret = 0, j;
char *prop_buf;
@@ -486,7 +503,21 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
if (ret)
goto out;
+ psy_for_each_extension(psy, ext) {
+ for (j = 0; j < ext->ext->num_properties; j++) {
+ if (test_bit(ext->ext->properties[j], psy_drv_properties))
+ continue;
+ set_bit(ext->ext->properties[j], psy_drv_properties);
+ ret = add_prop_uevent(dev, env, ext->ext->properties[j],
+ prop_buf);
+ if (ret)
+ goto out;
+ }
+ }
+
for (j = 0; j < psy->desc->num_properties; j++) {
+ if (test_bit(psy->desc->properties[j], psy_drv_properties))
+ continue;
set_bit(psy->desc->properties[j], psy_drv_properties);
ret = add_prop_uevent(dev, env, psy->desc->properties[j],
prop_buf);
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 8e5705a56b85..128e7a67f268 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -15,6 +15,7 @@
#include <linux/device.h>
#include <linux/workqueue.h>
#include <linux/leds.h>
+#include <linux/list.h>
#include <linux/spinlock.h>
#include <linux/notifier.h>
@@ -280,6 +281,25 @@ struct power_supply_desc {
int use_for_apm;
};
+struct power_supply_ext {
+ u8 charge_behaviours;
+ const enum power_supply_property *properties;
+ size_t num_properties;
+
+ int (*get_property)(struct power_supply *psy,
+ const struct power_supply_ext *ext,
+ enum power_supply_property psp,
+ union power_supply_propval *val);
+ int (*set_property)(struct power_supply *psy,
+ const struct power_supply_ext *ext,
+ enum power_supply_property psp,
+ const union power_supply_propval *val);
+
+ int (*property_is_writeable)(struct power_supply *psy,
+ const struct power_supply_ext *ext,
+ enum power_supply_property psp);
+};
+
struct power_supply {
const struct power_supply_desc *desc;
@@ -303,6 +323,7 @@ struct power_supply {
bool removing;
atomic_t use_cnt;
struct power_supply_battery_info *battery_info;
+ struct list_head extensions;
#ifdef CONFIG_THERMAL
struct thermal_zone_device *tzd;
struct thermal_cooling_device *tcd;
@@ -892,6 +913,11 @@ devm_power_supply_register_no_ws(struct device *parent,
extern void power_supply_unregister(struct power_supply *psy);
extern int power_supply_powers(struct power_supply *psy, struct device *dev);
+extern int power_supply_register_extension(struct power_supply *psy,
+ const struct power_supply_ext *ext);
+extern void power_supply_unregister_extension(struct power_supply *psy,
+ const struct power_supply_ext *ext);
+
#define to_power_supply(device) container_of(device, struct power_supply, dev)
extern void *power_supply_get_drvdata(struct power_supply *psy);
--
2.45.2
With the introduction of power supply extension, it will not be enough
to iterate the properties on the struct power_supply directly.
Instead introduce a helper power_supply_has_property() which ill handle
properties added by extensions.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
drivers/power/supply/power_supply.h | 3 ++
drivers/power/supply/power_supply_core.c | 10 +++----
drivers/power/supply/power_supply_hwmon.c | 48 +++++++++++++++----------------
drivers/power/supply/power_supply_sysfs.c | 14 +++------
4 files changed, 36 insertions(+), 39 deletions(-)
diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
index 3cbafc58bdad..622be1f0a180 100644
--- a/drivers/power/supply/power_supply.h
+++ b/drivers/power/supply/power_supply.h
@@ -13,6 +13,9 @@ struct device;
struct device_type;
struct power_supply;
+extern bool power_supply_has_property(const struct power_supply_desc *psy_desc,
+ enum power_supply_property psp);
+
#ifdef CONFIG_SYSFS
extern void power_supply_init_attrs(void);
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index fefe938c9342..d57ecdd966e0 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -1183,8 +1183,8 @@ bool power_supply_battery_bti_in_range(struct power_supply_battery_info *info,
}
EXPORT_SYMBOL_GPL(power_supply_battery_bti_in_range);
-static bool psy_has_property(const struct power_supply_desc *psy_desc,
- enum power_supply_property psp)
+bool power_supply_has_property(const struct power_supply_desc *psy_desc,
+ enum power_supply_property psp)
{
bool found = false;
int i;
@@ -1209,7 +1209,7 @@ int power_supply_get_property(struct power_supply *psy,
return -ENODEV;
}
- if (psy_has_property(psy->desc, psp))
+ if (power_supply_has_property(psy->desc, psp))
return psy->desc->get_property(psy, psp, val);
else if (power_supply_battery_info_has_prop(psy->battery_info, psp))
return power_supply_battery_info_get_prop(psy->battery_info, psp, val);
@@ -1308,7 +1308,7 @@ static int psy_register_thermal(struct power_supply *psy)
return 0;
/* Register battery zone device psy reports temperature */
- if (psy_has_property(psy->desc, POWER_SUPPLY_PROP_TEMP)) {
+ if (power_supply_has_property(psy->desc, POWER_SUPPLY_PROP_TEMP)) {
/* Prefer our hwmon device and avoid duplicates */
struct thermal_zone_params tzp = {
.no_hwmon = IS_ENABLED(CONFIG_POWER_SUPPLY_HWMON)
@@ -1361,7 +1361,7 @@ __power_supply_register(struct device *parent,
pr_warn("%s: Expected proper parent device for '%s'\n",
__func__, desc->name);
- if (psy_has_property(desc, POWER_SUPPLY_PROP_USB_TYPE) &&
+ if (power_supply_has_property(desc, POWER_SUPPLY_PROP_USB_TYPE) &&
(!desc->usb_types || !desc->num_usb_types))
return ERR_PTR(-EINVAL);
diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
index c97893d4c25e..2ecbe4a74c25 100644
--- a/drivers/power/supply/power_supply_hwmon.c
+++ b/drivers/power/supply/power_supply_hwmon.c
@@ -8,6 +8,8 @@
#include <linux/power_supply.h>
#include <linux/slab.h>
+#include "power_supply.h"
+
struct power_supply_hwmon {
struct power_supply *psy;
unsigned long *props;
@@ -324,9 +326,26 @@ static const struct hwmon_chip_info power_supply_hwmon_chip_info = {
.info = power_supply_hwmon_info,
};
+static const enum power_supply_property power_supply_hwmon_props[] = {
+ POWER_SUPPLY_PROP_CURRENT_AVG,
+ POWER_SUPPLY_PROP_CURRENT_MAX,
+ POWER_SUPPLY_PROP_CURRENT_NOW,
+ POWER_SUPPLY_PROP_TEMP,
+ POWER_SUPPLY_PROP_TEMP_MAX,
+ POWER_SUPPLY_PROP_TEMP_MIN,
+ POWER_SUPPLY_PROP_TEMP_ALERT_MIN,
+ POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
+ POWER_SUPPLY_PROP_TEMP_AMBIENT,
+ POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN,
+ POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX,
+ POWER_SUPPLY_PROP_VOLTAGE_AVG,
+ POWER_SUPPLY_PROP_VOLTAGE_MIN,
+ POWER_SUPPLY_PROP_VOLTAGE_MAX,
+ POWER_SUPPLY_PROP_VOLTAGE_NOW,
+};
+
int power_supply_add_hwmon_sysfs(struct power_supply *psy)
{
- const struct power_supply_desc *desc = psy->desc;
struct power_supply_hwmon *psyhw;
struct device *dev = &psy->dev;
struct device *hwmon;
@@ -352,30 +371,11 @@ int power_supply_add_hwmon_sysfs(struct power_supply *psy)
goto error;
}
- for (i = 0; i < desc->num_properties; i++) {
- const enum power_supply_property prop = desc->properties[i];
-
- switch (prop) {
- case POWER_SUPPLY_PROP_CURRENT_AVG:
- case POWER_SUPPLY_PROP_CURRENT_MAX:
- case POWER_SUPPLY_PROP_CURRENT_NOW:
- case POWER_SUPPLY_PROP_TEMP:
- case POWER_SUPPLY_PROP_TEMP_MAX:
- case POWER_SUPPLY_PROP_TEMP_MIN:
- case POWER_SUPPLY_PROP_TEMP_ALERT_MIN:
- case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
- case POWER_SUPPLY_PROP_TEMP_AMBIENT:
- case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN:
- case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX:
- case POWER_SUPPLY_PROP_VOLTAGE_AVG:
- case POWER_SUPPLY_PROP_VOLTAGE_MIN:
- case POWER_SUPPLY_PROP_VOLTAGE_MAX:
- case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+ for (i = 0; i < ARRAY_SIZE(power_supply_hwmon_props); i++) {
+ const enum power_supply_property prop = power_supply_hwmon_props[i];
+
+ if (power_supply_has_property(psy->desc, prop))
set_bit(prop, psyhw->props);
- break;
- default:
- break;
- }
}
name = psy->desc->name;
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 3e63d165b2f7..abd44ebfe6fe 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -367,7 +367,6 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
struct device *dev = kobj_to_dev(kobj);
struct power_supply *psy = dev_get_drvdata(dev);
umode_t mode = S_IRUSR | S_IRGRP | S_IROTH;
- int i;
if (!power_supply_attrs[attrno].prop_name)
return 0;
@@ -375,15 +374,10 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
if (attrno == POWER_SUPPLY_PROP_TYPE)
return mode;
- for (i = 0; i < psy->desc->num_properties; i++) {
- int property = psy->desc->properties[i];
-
- if (property == attrno) {
- if (power_supply_property_is_writeable(psy, property) > 0)
- mode |= S_IWUSR;
-
- return mode;
- }
+ if (power_supply_has_property(psy->desc, attrno)) {
+ if (power_supply_property_is_writeable(psy, attrno) > 0)
+ mode |= S_IWUSR;
+ return mode;
}
if (power_supply_battery_info_has_prop(psy->battery_info, attrno))
--
2.45.2
Signed-off-by: Thomas Weißschuh <[email protected]>
---
drivers/platform/x86/system76_acpi.c | 93 ++++++++++++++++++++----------------
1 file changed, 51 insertions(+), 42 deletions(-)
diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c
index 3da753b3d00d..d9bc5cae2dda 100644
--- a/drivers/platform/x86/system76_acpi.c
+++ b/drivers/platform/x86/system76_acpi.c
@@ -162,7 +162,7 @@ enum {
THRESHOLD_END,
};
-static ssize_t battery_get_threshold(int which, char *buf)
+static int battery_get_threshold(int which, int *val)
{
struct acpi_object_list input;
union acpi_object param;
@@ -186,29 +186,21 @@ static ssize_t battery_get_threshold(int which, char *buf)
if (ret == BATTERY_THRESHOLD_INVALID)
return -EINVAL;
- return sysfs_emit(buf, "%d\n", (int)ret);
+ *val = ret;
+ return 0;
}
-static ssize_t battery_set_threshold(int which, const char *buf, size_t count)
+static int battery_set_threshold(int which, unsigned int value)
{
struct acpi_object_list input;
union acpi_object params[2];
acpi_handle handle;
acpi_status status;
- unsigned int value;
- int ret;
handle = ec_get_handle();
if (!handle)
return -ENODEV;
- ret = kstrtouint(buf, 10, &value);
- if (ret)
- return ret;
-
- if (value > 100)
- return -EINVAL;
-
input.count = 2;
input.pointer = params;
// Start/stop selection
@@ -222,52 +214,69 @@ static ssize_t battery_set_threshold(int which, const char *buf, size_t count)
if (ACPI_FAILURE(status))
return -EIO;
- return count;
-}
-
-static ssize_t charge_control_start_threshold_show(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- return battery_get_threshold(THRESHOLD_START, buf);
-}
-
-static ssize_t charge_control_start_threshold_store(struct device *dev,
- struct device_attribute *attr, const char *buf, size_t count)
-{
- return battery_set_threshold(THRESHOLD_START, buf, count);
+ return 0;
}
-static DEVICE_ATTR_RW(charge_control_start_threshold);
+static const enum power_supply_property system76_battery_properties[] = {
+ POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD,
+ POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD,
+};
-static ssize_t charge_control_end_threshold_show(struct device *dev,
- struct device_attribute *attr, char *buf)
+static int system76_property_is_writeable(struct power_supply *psy,
+ const struct power_supply_ext *ext,
+ enum power_supply_property psp)
{
- return battery_get_threshold(THRESHOLD_END, buf);
+ return true;
}
-static ssize_t charge_control_end_threshold_store(struct device *dev,
- struct device_attribute *attr, const char *buf, size_t count)
+static int system76_get_property(struct power_supply *psy,
+ const struct power_supply_ext *ext,
+ enum power_supply_property psp,
+ union power_supply_propval *val)
{
- return battery_set_threshold(THRESHOLD_END, buf, count);
+ switch (psp) {
+ case POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD:
+ return battery_get_threshold(THRESHOLD_START, &val->intval);
+ case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
+ return battery_get_threshold(THRESHOLD_END, &val->intval);
+ default:
+ return -EINVAL;
+ };
+}
+
+static int system76_set_property(struct power_supply *psy, const struct power_supply_ext *ext,
+ enum power_supply_property psp,
+ const union power_supply_propval *val)
+{
+ switch (psp) {
+ case POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD:
+ if (val->intval < 0 || val->intval > 100)
+ return -EINVAL;
+ return battery_set_threshold(THRESHOLD_START, val->intval);
+ case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
+ if (val->intval < 0 || val->intval > 100)
+ return -EINVAL;
+ return battery_set_threshold(THRESHOLD_END, val->intval);
+ default:
+ return -EINVAL;
+ };
}
-static DEVICE_ATTR_RW(charge_control_end_threshold);
-
-static struct attribute *system76_battery_attrs[] = {
- &dev_attr_charge_control_start_threshold.attr,
- &dev_attr_charge_control_end_threshold.attr,
- NULL,
+static const struct power_supply_ext system76_power_supply_ext = {
+ .properties = system76_battery_properties,
+ .num_properties = ARRAY_SIZE(system76_battery_properties),
+ .property_is_writeable = system76_property_is_writeable,
+ .get_property = system76_get_property,
+ .set_property = system76_set_property,
};
-ATTRIBUTE_GROUPS(system76_battery);
-
static int system76_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
{
// System76 EC only supports 1 battery
if (strcmp(battery->desc->name, "BAT0") != 0)
return -ENODEV;
- if (device_add_groups(&battery->dev, system76_battery_groups))
+ if (power_supply_register_extension(battery, &system76_power_supply_ext))
return -ENODEV;
return 0;
@@ -275,7 +284,7 @@ static int system76_battery_add(struct power_supply *battery, struct acpi_batter
static int system76_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
{
- device_remove_groups(&battery->dev, system76_battery_groups);
+ power_supply_unregister_extension(battery, &system76_power_supply_ext);
return 0;
}
--
2.45.2
Am 08.06.24 um 21:19 schrieb Thomas Weißschuh:
> Signed-off-by: Thomas Weißschuh <[email protected]>
> ---
> drivers/power/supply/power_supply.h | 13 ++-
> drivers/power/supply/power_supply_core.c | 128 ++++++++++++++++++++++++++++--
> drivers/power/supply/power_supply_hwmon.c | 2 +-
> drivers/power/supply/power_supply_sysfs.c | 37 ++++++++-
> include/linux/power_supply.h | 26 ++++++
> 5 files changed, 192 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> index 622be1f0a180..686b66161900 100644
> --- a/drivers/power/supply/power_supply.h
> +++ b/drivers/power/supply/power_supply.h
> @@ -13,8 +13,17 @@ struct device;
> struct device_type;
> struct power_supply;
>
> -extern bool power_supply_has_property(const struct power_supply_desc *psy_desc,
> - enum power_supply_property psp);
> +struct psy_ext_registration {
> + struct list_head list_head;
> + const struct power_supply_ext *ext;
> +};
> +
> +#define psy_for_each_extension(psy, pos) list_for_each_entry(pos, &(psy)->extensions, list_head)
Hi,
sorry for taking so long to respond, the patch looks good to me except one single thing:
when removing a power supply extension, the driver has to be sure that no one is still using
the removed extension. So you might want to add some sort of locking when using a power supply
extension.
Thanks,
Armin Wolf
> +
> +bool power_supply_has_property(const struct power_supply *psy,
> + enum power_supply_property psp);
> +bool power_supply_ext_has_property(const struct power_supply_ext *psy_ext,
> + enum power_supply_property psp);
>
> #ifdef CONFIG_SYSFS
>
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index d57ecdd966e0..fcdfedf3df7c 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -1183,8 +1183,8 @@ bool power_supply_battery_bti_in_range(struct power_supply_battery_info *info,
> }
> EXPORT_SYMBOL_GPL(power_supply_battery_bti_in_range);
>
> -bool power_supply_has_property(const struct power_supply_desc *psy_desc,
> - enum power_supply_property psp)
> +static bool psy_desc_has_property(const struct power_supply_desc *psy_desc,
> + enum power_supply_property psp)
> {
> bool found = false;
> int i;
> @@ -1199,17 +1199,55 @@ bool power_supply_has_property(const struct power_supply_desc *psy_desc,
> return found;
> }
>
> +bool power_supply_ext_has_property(const struct power_supply_ext *psy_ext,
> + enum power_supply_property psp)
> +{
> + bool found = false;
> + int i;
> +
> + if (!psy_ext)
> + return false;
> +
> + for (i = 0; i < psy_ext->num_properties; i++) {
> + if (psy_ext->properties[i] == psp) {
> + found = true;
> + break;
> + }
> + }
> +
> + return found;
> +}
> +
> +bool power_supply_has_property(const struct power_supply *psy,
> + enum power_supply_property psp)
> +{
> + struct psy_ext_registration *reg;
> +
> + psy_for_each_extension(psy, reg)
> + if (power_supply_ext_has_property(reg->ext, psp))
> + return true;
> +
> + return psy_desc_has_property(psy->desc, psp);
> +}
> +
> int power_supply_get_property(struct power_supply *psy,
> enum power_supply_property psp,
> union power_supply_propval *val)
> {
> + struct psy_ext_registration *reg;
> +
> if (atomic_read(&psy->use_cnt) <= 0) {
> if (!psy->initialized)
> return -EAGAIN;
> return -ENODEV;
> }
>
> - if (power_supply_has_property(psy->desc, psp))
> + psy_for_each_extension(psy, reg) {
> + if (power_supply_ext_has_property(reg->ext, psp))
> + return reg->ext->get_property(psy, reg->ext, psp, val);
> + }
> +
> + if (psy_desc_has_property(psy->desc, psp))
> return psy->desc->get_property(psy, psp, val);
> else if (power_supply_battery_info_has_prop(psy->battery_info, psp))
> return power_supply_battery_info_get_prop(psy->battery_info, psp, val);
> @@ -1222,7 +1260,21 @@ int power_supply_set_property(struct power_supply *psy,
> enum power_supply_property psp,
> const union power_supply_propval *val)
> {
> - if (atomic_read(&psy->use_cnt) <= 0 || !psy->desc->set_property)
> + struct psy_ext_registration *reg;
> +
> + if (atomic_read(&psy->use_cnt) <= 0)
> + return -ENODEV;
> +
> + psy_for_each_extension(psy, reg) {
> + if (power_supply_ext_has_property(reg->ext, psp)) {
> + if (reg->ext->set_property)
> + return reg->ext->set_property(psy, reg->ext, psp, val);
> + else
> + return -ENODEV;
> + }
> + }
> +
> + if (!psy->desc->set_property)
> return -ENODEV;
>
> return psy->desc->set_property(psy, psp, val);
> @@ -1232,8 +1284,21 @@ EXPORT_SYMBOL_GPL(power_supply_set_property);
> int power_supply_property_is_writeable(struct power_supply *psy,
> enum power_supply_property psp)
> {
> - if (atomic_read(&psy->use_cnt) <= 0 ||
> - !psy->desc->property_is_writeable)
> + struct psy_ext_registration *reg;
> +
> + if (atomic_read(&psy->use_cnt) <= 0)
> + return -ENODEV;
> +
> + psy_for_each_extension(psy, reg) {
> + if (power_supply_ext_has_property(reg->ext, psp)) {
> + if (reg->ext->property_is_writeable)
> + return reg->ext->property_is_writeable(psy, reg->ext, psp);
> + else
> + return -ENODEV;
> + }
> + }
> +
> + if (!psy->desc->property_is_writeable)
> return -ENODEV;
>
> return psy->desc->property_is_writeable(psy, psp);
> @@ -1256,6 +1321,52 @@ int power_supply_powers(struct power_supply *psy, struct device *dev)
> }
> EXPORT_SYMBOL_GPL(power_supply_powers);
>
> +static int power_supply_update_groups(struct power_supply *psy)
> +{
> + int ret;
> +
> + ret = sysfs_update_groups(&psy->dev.kobj, power_supply_dev_type.groups);
> + power_supply_changed(psy);
> + return ret;
> +}
> +
> +int power_supply_register_extension(struct power_supply *psy, const struct power_supply_ext *ext)
> +{
> + struct psy_ext_registration *reg;
> + size_t i;
> +
> + for (i = 0; i < ext->num_properties; i++) {
> + if (power_supply_has_property(psy, ext->properties[i]))
> + return -EEXIST;
> + }
> +
> + reg = devm_kmalloc(&psy->dev, sizeof(*reg), GFP_KERNEL);
> + if (!reg)
> + return -ENOMEM;
> +
> + reg->ext = ext;
> + list_add(®->list_head, &psy->extensions);
> +
> + return power_supply_update_groups(psy);
> +}
> +EXPORT_SYMBOL_GPL(power_supply_register_extension);
> +
> +void power_supply_unregister_extension(struct power_supply *psy, const struct power_supply_ext *ext)
> +{
> + struct psy_ext_registration *reg;
> +
> + psy_for_each_extension(psy, reg) {
> + if (reg->ext == ext) {
> + list_del(®->list_head);
> + power_supply_update_groups(psy);
> + return;
> + }
> + }
> +
> + dev_warn(&psy->dev, "Trying to unregister invalid extension");
> +}
> +EXPORT_SYMBOL_GPL(power_supply_unregister_extension);
> +
> static void power_supply_dev_release(struct device *dev)
> {
> struct power_supply *psy = to_power_supply(dev);
> @@ -1308,7 +1419,7 @@ static int psy_register_thermal(struct power_supply *psy)
> return 0;
>
> /* Register battery zone device psy reports temperature */
> - if (power_supply_has_property(psy->desc, POWER_SUPPLY_PROP_TEMP)) {
> + if (power_supply_has_property(psy, POWER_SUPPLY_PROP_TEMP)) {
> /* Prefer our hwmon device and avoid duplicates */
> struct thermal_zone_params tzp = {
> .no_hwmon = IS_ENABLED(CONFIG_POWER_SUPPLY_HWMON)
> @@ -1361,7 +1472,7 @@ __power_supply_register(struct device *parent,
> pr_warn("%s: Expected proper parent device for '%s'\n",
> __func__, desc->name);
>
> - if (power_supply_has_property(desc, POWER_SUPPLY_PROP_USB_TYPE) &&
> + if (psy_desc_has_property(desc, POWER_SUPPLY_PROP_USB_TYPE) &&
> (!desc->usb_types || !desc->num_usb_types))
> return ERR_PTR(-EINVAL);
>
> @@ -1415,6 +1526,7 @@ __power_supply_register(struct device *parent,
> }
>
> spin_lock_init(&psy->changed_lock);
> + INIT_LIST_HEAD(&psy->extensions);
> rc = device_add(dev);
> if (rc)
> goto device_add_failed;
> diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
> index 2ecbe4a74c25..8cb852a734b1 100644
> --- a/drivers/power/supply/power_supply_hwmon.c
> +++ b/drivers/power/supply/power_supply_hwmon.c
> @@ -374,7 +374,7 @@ int power_supply_add_hwmon_sysfs(struct power_supply *psy)
> for (i = 0; i < ARRAY_SIZE(power_supply_hwmon_props); i++) {
> const enum power_supply_property prop = power_supply_hwmon_props[i];
>
> - if (power_supply_has_property(psy->desc, prop))
> + if (power_supply_has_property(psy, prop))
> set_bit(prop, psyhw->props);
> }
>
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index abd44ebfe6fe..8f29ddea08d0 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -271,6 +271,23 @@ static ssize_t power_supply_show_usb_type(struct device *dev,
> return count;
> }
>
> +static ssize_t power_supply_show_charge_behaviour(struct device *dev,
> + struct power_supply *psy,
> + union power_supply_propval *value,
> + char *buf)
> +{
> + struct psy_ext_registration *reg;
> +
> + psy_for_each_extension(psy, reg) {
> + if (power_supply_ext_has_property(reg->ext, POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR))
> + return power_supply_charge_behaviour_show(dev, reg->ext->charge_behaviours,
> + value->intval, buf);
> + }
> +
> + return power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours,
> + value->intval, buf);
> +}
> +
> static ssize_t power_supply_show_property(struct device *dev,
> struct device_attribute *attr,
> char *buf) {
> @@ -304,8 +321,7 @@ static ssize_t power_supply_show_property(struct device *dev,
> &value, buf);
> break;
> case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
> - ret = power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours,
> - value.intval, buf);
> + ret = power_supply_show_charge_behaviour(dev, psy, &value, buf);
> break;
> case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER:
> ret = sysfs_emit(buf, "%s\n", value.strval);
> @@ -374,7 +390,7 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
> if (attrno == POWER_SUPPLY_PROP_TYPE)
> return mode;
>
> - if (power_supply_has_property(psy->desc, attrno)) {
> + if (power_supply_has_property(psy, attrno)) {
> if (power_supply_property_is_writeable(psy, attrno) > 0)
> mode |= S_IWUSR;
> return mode;
> @@ -459,6 +475,7 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
> power_supply_battery_info_properties;
> unsigned long psy_drv_properties[POWER_SUPPLY_ATTR_CNT /
> sizeof(unsigned long) + 1] = {0};
> + struct psy_ext_registration *ext;
> int ret = 0, j;
> char *prop_buf;
>
> @@ -486,7 +503,21 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
> if (ret)
> goto out;
>
> + psy_for_each_extension(psy, ext) {
> + for (j = 0; j < ext->ext->num_properties; j++) {
> + if (test_bit(ext->ext->properties[j], psy_drv_properties))
> + continue;
> + set_bit(ext->ext->properties[j], psy_drv_properties);
> + ret = add_prop_uevent(dev, env, ext->ext->properties[j],
> + prop_buf);
> + if (ret)
> + goto out;
> + }
> + }
> +
> for (j = 0; j < psy->desc->num_properties; j++) {
> + if (test_bit(psy->desc->properties[j], psy_drv_properties))
> + continue;
> set_bit(psy->desc->properties[j], psy_drv_properties);
> ret = add_prop_uevent(dev, env, psy->desc->properties[j],
> prop_buf);
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 8e5705a56b85..128e7a67f268 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -15,6 +15,7 @@
> #include <linux/device.h>
> #include <linux/workqueue.h>
> #include <linux/leds.h>
> +#include <linux/list.h>
> #include <linux/spinlock.h>
> #include <linux/notifier.h>
>
> @@ -280,6 +281,25 @@ struct power_supply_desc {
> int use_for_apm;
> };
>
> +struct power_supply_ext {
> + u8 charge_behaviours;
> + const enum power_supply_property *properties;
> + size_t num_properties;
> +
> + int (*get_property)(struct power_supply *psy,
> + const struct power_supply_ext *ext,
> + enum power_supply_property psp,
> + union power_supply_propval *val);
> + int (*set_property)(struct power_supply *psy,
> + const struct power_supply_ext *ext,
> + enum power_supply_property psp,
> + const union power_supply_propval *val);
> +
> + int (*property_is_writeable)(struct power_supply *psy,
> + const struct power_supply_ext *ext,
> + enum power_supply_property psp);
> +};
> +
> struct power_supply {
> const struct power_supply_desc *desc;
>
> @@ -303,6 +323,7 @@ struct power_supply {
> bool removing;
> atomic_t use_cnt;
> struct power_supply_battery_info *battery_info;
> + struct list_head extensions;
> #ifdef CONFIG_THERMAL
> struct thermal_zone_device *tzd;
> struct thermal_cooling_device *tcd;
> @@ -892,6 +913,11 @@ devm_power_supply_register_no_ws(struct device *parent,
> extern void power_supply_unregister(struct power_supply *psy);
> extern int power_supply_powers(struct power_supply *psy, struct device *dev);
>
> +extern int power_supply_register_extension(struct power_supply *psy,
> + const struct power_supply_ext *ext);
> +extern void power_supply_unregister_extension(struct power_supply *psy,
> + const struct power_supply_ext *ext);
> +
> #define to_power_supply(device) container_of(device, struct power_supply, dev)
>
> extern void *power_supply_get_drvdata(struct power_supply *psy);
>
Hi!
On 2024-06-14 01:11:29+0000, Armin Wolf wrote:
> Am 08.06.24 um 21:19 schrieb Thomas Weißschuh:
>
> > Signed-off-by: Thomas Weißschuh <[email protected]>
> > ---
> > drivers/power/supply/power_supply.h | 13 ++-
> > drivers/power/supply/power_supply_core.c | 128 ++++++++++++++++++++++++++++--
> > drivers/power/supply/power_supply_hwmon.c | 2 +-
> > drivers/power/supply/power_supply_sysfs.c | 37 ++++++++-
> > include/linux/power_supply.h | 26 ++++++
> > 5 files changed, 192 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> > index 622be1f0a180..686b66161900 100644
> > --- a/drivers/power/supply/power_supply.h
> > +++ b/drivers/power/supply/power_supply.h
> > @@ -13,8 +13,17 @@ struct device;
> > struct device_type;
> > struct power_supply;
> >
> > -extern bool power_supply_has_property(const struct power_supply_desc *psy_desc,
> > - enum power_supply_property psp);
> > +struct psy_ext_registration {
> > + struct list_head list_head;
> > + const struct power_supply_ext *ext;
> > +};
> > +
> > +#define psy_for_each_extension(psy, pos) list_for_each_entry(pos, &(psy)->extensions, list_head)
>
> sorry for taking so long to respond, the patch looks good to me except one single thing:
>
> when removing a power supply extension, the driver has to be sure that no one is still using
> the removed extension. So you might want to add some sort of locking when using a power supply
> extension.
That was part of the v1 of the series [0].
I dropped it in v2 to focus on the external API and semantics first.
IMO now that we have the loop macro anyways that can also take care of
the locking: psy_for_each_extension_locked()
[0] https://lore.kernel.org/all/20240606-power-supply-extensions-v1-4-b45669290bdc@weissschuh.net/
Thomas
<snip>