Hi,
This series cleans up the generic ADC battery driver and adds
devicetree support. The plan is to use the driver to add upstream
support for a handheld thermal camera.
Instead of reading and exposing the monitored battery data manually
I started the series with an addition to the power-supply core,
which allows automatic handling of the static battery information.
It simplifies the generic-adc-battery driver a lot and should also
be useful for other battery drivers.
Changes since PATCHv1:
* collect Reviewed-by
(I did not collect them for the auto-exposure because of the
code changes)
* always auto expose battery data (without opt-in)
* update DT binding according to feedback
* add temperature support
* fix issues pointed out by the Intel build bot
- move power_supply_battery_info_properties to power_supply_core.c
- restore accidently removed EXPORT_SYMBOL for power_supply_get_property
-- Sebastian
Sebastian Reichel (12):
dt-bindings: power: supply: adc-battery: add binding
power: supply: core: auto-exposure of simple-battery data
power: supply: generic-adc-battery: convert to managed resources
power: supply: generic-adc-battery: fix unit scaling
power: supply: generic-adc-battery: drop jitter delay support
power: supply: generic-adc-battery: drop charge now support
power: supply: generic-adc-battery: drop memory alloc error message
power: supply: generic-adc-battery: use simple-battery API
power: supply: generic-adc-battery: simplify read_channel logic
power: supply: generic-adc-battery: add temperature support
power: supply: generic-adc-battery: add DT support
power: supply: generic-adc-battery: update copyright info
.../bindings/power/supply/adc-battery.yaml | 70 ++++++
drivers/power/supply/generic-adc-battery.c | 227 +++++-------------
drivers/power/supply/power_supply_core.c | 173 +++++++++++--
drivers/power/supply/power_supply_sysfs.c | 15 ++
include/linux/power/generic-adc-battery.h | 23 --
include/linux/power_supply.h | 8 +
6 files changed, 306 insertions(+), 210 deletions(-)
create mode 100644 Documentation/devicetree/bindings/power/supply/adc-battery.yaml
delete mode 100644 include/linux/power/generic-adc-battery.h
--
2.39.2
Add support for automatically exposing data from the
simple-battery firmware node with a single configuration
option in the power-supply device.
Signed-off-by: Sebastian Reichel <[email protected]>
---
drivers/power/supply/power_supply_core.c | 173 +++++++++++++++++++---
drivers/power/supply/power_supply_sysfs.c | 15 ++
include/linux/power_supply.h | 8 +
3 files changed, 178 insertions(+), 18 deletions(-)
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index f3d7c1da299f..842c27de4fac 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -388,7 +388,7 @@ static int __power_supply_get_supplier_property(struct device *dev, void *_data)
struct psy_get_supplier_prop_data *data = _data;
if (__power_supply_is_supplied_by(epsy, data->psy))
- if (!epsy->desc->get_property(epsy, data->psp, data->val))
+ if (!power_supply_get_property(epsy, data->psp, data->val))
return 1; /* Success */
return 0; /* Continue iterating */
@@ -832,6 +832,133 @@ void power_supply_put_battery_info(struct power_supply *psy,
}
EXPORT_SYMBOL_GPL(power_supply_put_battery_info);
+const enum power_supply_property power_supply_battery_info_properties[] = {
+ POWER_SUPPLY_PROP_TECHNOLOGY,
+ POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
+ POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
+ POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
+ POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
+ POWER_SUPPLY_PROP_PRECHARGE_CURRENT,
+ POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT,
+ POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
+ POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
+ POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN,
+ POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX,
+ POWER_SUPPLY_PROP_TEMP_ALERT_MIN,
+ POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
+ POWER_SUPPLY_PROP_TEMP_MIN,
+ POWER_SUPPLY_PROP_TEMP_MAX,
+};
+EXPORT_SYMBOL_GPL(power_supply_battery_info_properties);
+
+const size_t power_supply_battery_info_properties_size = ARRAY_SIZE(power_supply_battery_info_properties);
+EXPORT_SYMBOL_GPL(power_supply_battery_info_properties_size);
+
+bool power_supply_battery_info_has_prop(struct power_supply_battery_info *info,
+ enum power_supply_property psp)
+{
+ if (!info)
+ return false;
+
+ switch (psp) {
+ case POWER_SUPPLY_PROP_TECHNOLOGY:
+ return info->technology != POWER_SUPPLY_TECHNOLOGY_UNKNOWN;
+ case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN:
+ return info->energy_full_design_uwh >= 0;
+ case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
+ return info->charge_full_design_uah >= 0;
+ case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
+ return info->voltage_min_design_uv >= 0;
+ case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
+ return info->voltage_max_design_uv >= 0;
+ case POWER_SUPPLY_PROP_PRECHARGE_CURRENT:
+ return info->precharge_current_ua >= 0;
+ case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT:
+ return info->charge_term_current_ua >= 0;
+ case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
+ return info->constant_charge_current_max_ua >= 0;
+ case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
+ return info->constant_charge_voltage_max_uv >= 0;
+ case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN:
+ return info->temp_ambient_alert_min > INT_MIN;
+ case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX:
+ return info->temp_ambient_alert_max < INT_MAX;
+ case POWER_SUPPLY_PROP_TEMP_ALERT_MIN:
+ return info->temp_alert_min > INT_MIN;
+ case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
+ return info->temp_alert_max < INT_MAX;
+ case POWER_SUPPLY_PROP_TEMP_MIN:
+ return info->temp_min > INT_MIN;
+ case POWER_SUPPLY_PROP_TEMP_MAX:
+ return info->temp_max < INT_MAX;
+ default:
+ return false;
+ }
+}
+EXPORT_SYMBOL_GPL(power_supply_battery_info_has_prop);
+
+int power_supply_battery_info_get_prop(struct power_supply_battery_info *info,
+ enum power_supply_property psp,
+ union power_supply_propval *val)
+{
+ if (!info)
+ return -EINVAL;
+
+ if (!power_supply_battery_info_has_prop(info, psp))
+ return -EINVAL;
+
+ switch (psp) {
+ case POWER_SUPPLY_PROP_TECHNOLOGY:
+ val->intval = info->technology;
+ return 0;
+ case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN:
+ val->intval = info->energy_full_design_uwh;
+ return 0;
+ case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
+ val->intval = info->charge_full_design_uah;
+ return 0;
+ case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
+ val->intval = info->voltage_min_design_uv;
+ return 0;
+ case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
+ val->intval = info->voltage_max_design_uv;
+ return 0;
+ case POWER_SUPPLY_PROP_PRECHARGE_CURRENT:
+ val->intval = info->precharge_current_ua;
+ return 0;
+ case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT:
+ val->intval = info->charge_term_current_ua;
+ return 0;
+ case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
+ val->intval = info->constant_charge_current_max_ua;
+ return 0;
+ case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
+ val->intval = info->constant_charge_voltage_max_uv;
+ return 0;
+ case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN:
+ val->intval = info->temp_ambient_alert_min;
+ return 0;
+ case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX:
+ val->intval = info->temp_ambient_alert_max;
+ return 0;
+ case POWER_SUPPLY_PROP_TEMP_ALERT_MIN:
+ val->intval = info->temp_alert_min;
+ return 0;
+ case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
+ val->intval = info->temp_alert_max;
+ return 0;
+ case POWER_SUPPLY_PROP_TEMP_MIN:
+ val->intval = info->temp_min;
+ return 0;
+ case POWER_SUPPLY_PROP_TEMP_MAX:
+ val->intval = info->temp_max;
+ return 0;
+ default:
+ return -EINVAL;
+ }
+}
+EXPORT_SYMBOL_GPL(power_supply_battery_info_get_prop);
+
/**
* power_supply_temp2resist_simple() - find the battery internal resistance
* percent from temperature
@@ -1046,6 +1173,22 @@ 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 found = false;
+ int i;
+
+ for (i = 0; i < psy_desc->num_properties; i++) {
+ if (psy_desc->properties[i] == psp) {
+ found = true;
+ break;
+ }
+ }
+
+ return found;
+}
+
int power_supply_get_property(struct power_supply *psy,
enum power_supply_property psp,
union power_supply_propval *val)
@@ -1056,7 +1199,12 @@ int power_supply_get_property(struct power_supply *psy,
return -ENODEV;
}
- return psy->desc->get_property(psy, psp, val);
+ if (psy_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);
+ else
+ return -EINVAL;
}
EXPORT_SYMBOL_GPL(power_supply_get_property);
@@ -1117,22 +1265,6 @@ void power_supply_unreg_notifier(struct notifier_block *nb)
}
EXPORT_SYMBOL_GPL(power_supply_unreg_notifier);
-static bool psy_has_property(const struct power_supply_desc *psy_desc,
- enum power_supply_property psp)
-{
- bool found = false;
- int i;
-
- for (i = 0; i < psy_desc->num_properties; i++) {
- if (psy_desc->properties[i] == psp) {
- found = true;
- break;
- }
- }
-
- return found;
-}
-
#ifdef CONFIG_THERMAL
static int power_supply_read_temp(struct thermal_zone_device *tzd,
int *temp)
@@ -1255,6 +1387,11 @@ __power_supply_register(struct device *parent,
goto check_supplies_failed;
}
+ /* psy->battery_info is optional */
+ rc = power_supply_get_battery_info(psy, &psy->battery_info);
+ if (rc && rc != -ENODEV)
+ goto check_supplies_failed;
+
spin_lock_init(&psy->changed_lock);
rc = device_add(dev);
if (rc)
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index c228205e0953..5842dfe5dfb7 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -380,6 +380,9 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
}
}
+ if (power_supply_battery_info_has_prop(psy->battery_info, attrno))
+ return mode;
+
return 0;
}
@@ -461,6 +464,8 @@ static int add_prop_uevent(const struct device *dev, struct kobj_uevent_env *env
int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
{
const struct power_supply *psy = dev_get_drvdata(dev);
+ const enum power_supply_property *battery_props =
+ power_supply_battery_info_properties;
int ret = 0, j;
char *prop_buf;
@@ -488,6 +493,16 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
goto out;
}
+ for (j = 0; j < power_supply_battery_info_properties_size; j++) {
+ if (!power_supply_battery_info_has_prop(psy->battery_info,
+ battery_props[j]))
+ continue;
+ ret = add_prop_uevent(dev, env, battery_props[j],
+ prop_buf);
+ if (ret)
+ goto out;
+ }
+
out:
free_page((unsigned long)prop_buf);
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index aa2c4a7c4826..a427f13c757f 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -301,6 +301,7 @@ struct power_supply {
bool initialized;
bool removing;
atomic_t use_cnt;
+ struct power_supply_battery_info *battery_info;
#ifdef CONFIG_THERMAL
struct thermal_zone_device *tzd;
struct thermal_cooling_device *tcd;
@@ -791,10 +792,17 @@ devm_power_supply_get_by_phandle(struct device *dev, const char *property)
{ return NULL; }
#endif /* CONFIG_OF */
+extern const enum power_supply_property power_supply_battery_info_properties[];
+extern const size_t power_supply_battery_info_properties_size;
extern int power_supply_get_battery_info(struct power_supply *psy,
struct power_supply_battery_info **info_out);
extern void power_supply_put_battery_info(struct power_supply *psy,
struct power_supply_battery_info *info);
+extern bool power_supply_battery_info_has_prop(struct power_supply_battery_info *info,
+ enum power_supply_property psp);
+extern int power_supply_battery_info_get_prop(struct power_supply_battery_info *info,
+ enum power_supply_property psp,
+ union power_supply_propval *val);
extern int power_supply_ocv2cap_simple(struct power_supply_battery_ocv_table *table,
int table_len, int ocv);
extern struct power_supply_battery_ocv_table *
--
2.39.2
power-supply properties are reported in µV, µA and µW.
The IIO API provides mV, mA, mW, so the values need to
be multiplied by 1000.
Fixes: e60fea794e6e ("power: battery: Generic battery driver using IIO")
Reviewed-by: Linus Walleij <[email protected]>
Reviewed-by: Matti Vaittinen <[email protected]>
Signed-off-by: Sebastian Reichel <[email protected]>
---
drivers/power/supply/generic-adc-battery.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/power/supply/generic-adc-battery.c b/drivers/power/supply/generic-adc-battery.c
index 917bd2a6cc52..535972a332b3 100644
--- a/drivers/power/supply/generic-adc-battery.c
+++ b/drivers/power/supply/generic-adc-battery.c
@@ -136,6 +136,9 @@ static int read_channel(struct gab *adc_bat, enum power_supply_property psp,
result);
if (ret < 0)
pr_err("read channel error\n");
+ else
+ *result *= 1000;
+
return ret;
}
--
2.39.2
Convert driver to use managed resources to simplify driver code.
Reviewed-by: Linus Walleij <[email protected]>
Reviewed-by: Matti Vaittinen <[email protected]>
Signed-off-by: Sebastian Reichel <[email protected]>
---
drivers/power/supply/generic-adc-battery.c | 81 ++++++----------------
1 file changed, 23 insertions(+), 58 deletions(-)
diff --git a/drivers/power/supply/generic-adc-battery.c b/drivers/power/supply/generic-adc-battery.c
index 66039c665dd1..917bd2a6cc52 100644
--- a/drivers/power/supply/generic-adc-battery.c
+++ b/drivers/power/supply/generic-adc-battery.c
@@ -23,6 +23,7 @@
#include <linux/iio/consumer.h>
#include <linux/iio/types.h>
#include <linux/power/generic-adc-battery.h>
+#include <linux/devm-helpers.h>
#define JITTER_DEFAULT 10 /* hope 10ms is enough */
@@ -266,14 +267,13 @@ static int gab_probe(struct platform_device *pdev)
* copying the static properties and allocating extra memory for holding
* the extra configurable properties received from platform data.
*/
- properties = kcalloc(ARRAY_SIZE(gab_props) +
- ARRAY_SIZE(gab_chan_name),
- sizeof(*properties),
- GFP_KERNEL);
- if (!properties) {
- ret = -ENOMEM;
- goto first_mem_fail;
- }
+ properties = devm_kcalloc(&pdev->dev,
+ ARRAY_SIZE(gab_props) +
+ ARRAY_SIZE(gab_chan_name),
+ sizeof(*properties),
+ GFP_KERNEL);
+ if (!properties)
+ return -ENOMEM;
memcpy(properties, gab_props, sizeof(gab_props));
@@ -282,12 +282,13 @@ static int gab_probe(struct platform_device *pdev)
* based on the channel supported by consumer device.
*/
for (chan = 0; chan < ARRAY_SIZE(gab_chan_name); chan++) {
- adc_bat->channel[chan] = iio_channel_get(&pdev->dev,
- gab_chan_name[chan]);
+ adc_bat->channel[chan] = devm_iio_channel_get(&pdev->dev, gab_chan_name[chan]);
if (IS_ERR(adc_bat->channel[chan])) {
ret = PTR_ERR(adc_bat->channel[chan]);
+ if (ret != -ENODEV)
+ return dev_err_probe(&pdev->dev, ret, "Failed to get ADC channel %s\n", gab_chan_name[chan]);
adc_bat->channel[chan] = NULL;
- } else {
+ } else if (adc_bat->channel[chan]) {
/* copying properties for supported channels only */
int index2;
@@ -302,10 +303,8 @@ static int gab_probe(struct platform_device *pdev)
}
/* none of the channels are supported so let's bail out */
- if (!any) {
- ret = -ENODEV;
- goto second_mem_fail;
- }
+ if (!any)
+ return dev_err_probe(&pdev->dev, -ENODEV, "Failed to get any ADC channel\n");
/*
* Total number of properties is equal to static properties
@@ -316,25 +315,24 @@ static int gab_probe(struct platform_device *pdev)
psy_desc->properties = properties;
psy_desc->num_properties = index;
- adc_bat->psy = power_supply_register(&pdev->dev, psy_desc, &psy_cfg);
- if (IS_ERR(adc_bat->psy)) {
- ret = PTR_ERR(adc_bat->psy);
- goto err_reg_fail;
- }
+ adc_bat->psy = devm_power_supply_register(&pdev->dev, psy_desc, &psy_cfg);
+ if (IS_ERR(adc_bat->psy))
+ return dev_err_probe(&pdev->dev, PTR_ERR(adc_bat->psy), "Failed to register power-supply device\n");
- INIT_DELAYED_WORK(&adc_bat->bat_work, gab_work);
+ ret = devm_delayed_work_autocancel(&pdev->dev, &adc_bat->bat_work, gab_work);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret, "Failed to register delayed work\n");
- adc_bat->charge_finished = devm_gpiod_get_optional(&pdev->dev,
- "charged", GPIOD_IN);
+ adc_bat->charge_finished = devm_gpiod_get_optional(&pdev->dev, "charged", GPIOD_IN);
if (adc_bat->charge_finished) {
int irq;
irq = gpiod_to_irq(adc_bat->charge_finished);
- ret = request_any_context_irq(irq, gab_charged,
+ ret = devm_request_any_context_irq(&pdev->dev, irq, gab_charged,
IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
"battery charged", adc_bat);
if (ret < 0)
- goto gpio_req_fail;
+ return dev_err_probe(&pdev->dev, ret, "Failed to register irq\n");
}
platform_set_drvdata(pdev, adc_bat);
@@ -343,38 +341,6 @@ static int gab_probe(struct platform_device *pdev)
schedule_delayed_work(&adc_bat->bat_work,
msecs_to_jiffies(0));
return 0;
-
-gpio_req_fail:
- power_supply_unregister(adc_bat->psy);
-err_reg_fail:
- for (chan = 0; chan < ARRAY_SIZE(gab_chan_name); chan++) {
- if (adc_bat->channel[chan])
- iio_channel_release(adc_bat->channel[chan]);
- }
-second_mem_fail:
- kfree(properties);
-first_mem_fail:
- return ret;
-}
-
-static int gab_remove(struct platform_device *pdev)
-{
- int chan;
- struct gab *adc_bat = platform_get_drvdata(pdev);
-
- power_supply_unregister(adc_bat->psy);
-
- if (adc_bat->charge_finished)
- free_irq(gpiod_to_irq(adc_bat->charge_finished), adc_bat);
-
- for (chan = 0; chan < ARRAY_SIZE(gab_chan_name); chan++) {
- if (adc_bat->channel[chan])
- iio_channel_release(adc_bat->channel[chan]);
- }
-
- kfree(adc_bat->psy_desc.properties);
- cancel_delayed_work_sync(&adc_bat->bat_work);
- return 0;
}
static int __maybe_unused gab_suspend(struct device *dev)
@@ -408,7 +374,6 @@ static struct platform_driver gab_driver = {
.pm = &gab_pm_ops,
},
.probe = gab_probe,
- .remove = gab_remove,
};
module_platform_driver(gab_driver);
--
2.39.2
Add binding for a battery that is only monitored via ADC
channels and simple status GPIOs.
Signed-off-by: Sebastian Reichel <[email protected]>
---
.../bindings/power/supply/adc-battery.yaml | 70 +++++++++++++++++++
1 file changed, 70 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power/supply/adc-battery.yaml
diff --git a/Documentation/devicetree/bindings/power/supply/adc-battery.yaml b/Documentation/devicetree/bindings/power/supply/adc-battery.yaml
new file mode 100644
index 000000000000..ed9702caedff
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/adc-battery.yaml
@@ -0,0 +1,70 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/supply/adc-battery.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ADC battery
+
+maintainers:
+ - Sebastian Reichel <[email protected]>
+
+description:
+ Basic battery capacity meter, which only reports basic battery data
+ via ADC channels and optionally indicate that the battery is full by
+ polling a GPIO line.
+
+ The voltage is expected to be measured between the battery terminals
+ and mandatory. The optional current/power channel is expected to
+ monitor the current/power flowing out of the battery. Last but not
+ least the temperature channel is supposed to measure the battery
+ temperature.
+
+allOf:
+ - $ref: power-supply.yaml#
+
+properties:
+ compatible:
+ const: adc-battery
+
+ charged-gpios:
+ description:
+ GPIO which signals that the battery is fully charged. The GPIO is
+ often provided by charger ICs, that are not software controllable.
+ maxItems: 1
+
+ io-channels:
+ minItems: 1
+ maxItems: 4
+
+ io-channel-names:
+ minItems: 1
+ items:
+ - const: voltage
+ - enum: [ current, power, temperature ]
+ - enum: [ power, temperature ]
+ - const: temperature
+
+ monitored-battery: true
+
+required:
+ - compatible
+ - io-channels
+ - io-channel-names
+ - monitored-battery
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+
+ fuel-gauge {
+ compatible = "adc-battery";
+ charged-gpios = <&gpio 42 GPIO_ACTIVE_HIGH>;
+ io-channels = <&adc 13>, <&adc 37>;
+ io-channel-names = "voltage", "current";
+
+ power-supplies = <&charger>;
+ monitored-battery = <&battery>;
+ };
--
2.39.2
This adds full DT support to the driver. Because of the previous
changes just adding a compatible value is enough.
Reviewed-by: Linus Walleij <[email protected]>
Reviewed-by: Matti Vaittinen <[email protected]>
Signed-off-by: Sebastian Reichel <[email protected]>
---
drivers/power/supply/generic-adc-battery.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/power/supply/generic-adc-battery.c b/drivers/power/supply/generic-adc-battery.c
index 0124d8d51af7..e11ad43ab968 100644
--- a/drivers/power/supply/generic-adc-battery.c
+++ b/drivers/power/supply/generic-adc-battery.c
@@ -22,6 +22,7 @@
#include <linux/slab.h>
#include <linux/iio/consumer.h>
#include <linux/iio/types.h>
+#include <linux/of.h>
#include <linux/devm-helpers.h>
#define JITTER_DEFAULT 10 /* hope 10ms is enough */
@@ -175,6 +176,7 @@ static int gab_probe(struct platform_device *pdev)
if (!adc_bat)
return -ENOMEM;
+ psy_cfg.of_node = pdev->dev.of_node;
psy_cfg.drv_data = adc_bat;
psy_desc = &adc_bat->psy_desc;
psy_desc->name = dev_name(&pdev->dev);
@@ -288,10 +290,17 @@ static int __maybe_unused gab_resume(struct device *dev)
static SIMPLE_DEV_PM_OPS(gab_pm_ops, gab_suspend, gab_resume);
+static const struct of_device_id gab_match[] = {
+ { .compatible = "adc-battery" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, gab_match);
+
static struct platform_driver gab_driver = {
.driver = {
.name = "generic-adc-battery",
.pm = &gab_pm_ops,
+ .of_match_table = gab_match,
},
.probe = gab_probe,
};
--
2.39.2
Drop CHARGE_NOW support, which requires a platform specific
calculation method.
Reviewed-by: Linus Walleij <[email protected]>
Reviewed-by: Matti Vaittinen <[email protected]>
Signed-off-by: Sebastian Reichel <[email protected]>
---
drivers/power/supply/generic-adc-battery.c | 4 ----
include/linux/power/generic-adc-battery.h | 2 --
2 files changed, 6 deletions(-)
diff --git a/drivers/power/supply/generic-adc-battery.c b/drivers/power/supply/generic-adc-battery.c
index e20894460d7f..d07eeb7d46d3 100644
--- a/drivers/power/supply/generic-adc-battery.c
+++ b/drivers/power/supply/generic-adc-battery.c
@@ -72,7 +72,6 @@ static const enum power_supply_property gab_props[] = {
POWER_SUPPLY_PROP_STATUS,
POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
POWER_SUPPLY_PROP_CHARGE_EMPTY_DESIGN,
- POWER_SUPPLY_PROP_CHARGE_NOW,
POWER_SUPPLY_PROP_VOLTAGE_NOW,
POWER_SUPPLY_PROP_CURRENT_NOW,
POWER_SUPPLY_PROP_TECHNOLOGY,
@@ -166,9 +165,6 @@ static int gab_get_property(struct power_supply *psy,
case POWER_SUPPLY_PROP_CHARGE_EMPTY_DESIGN:
val->intval = 0;
break;
- case POWER_SUPPLY_PROP_CHARGE_NOW:
- val->intval = pdata->cal_charge(result);
- break;
case POWER_SUPPLY_PROP_VOLTAGE_NOW:
case POWER_SUPPLY_PROP_CURRENT_NOW:
case POWER_SUPPLY_PROP_POWER_NOW:
diff --git a/include/linux/power/generic-adc-battery.h b/include/linux/power/generic-adc-battery.h
index 50eb4bf28286..54434e4304d3 100644
--- a/include/linux/power/generic-adc-battery.h
+++ b/include/linux/power/generic-adc-battery.h
@@ -10,11 +10,9 @@
* struct gab_platform_data - platform_data for generic adc iio battery driver.
* @battery_info: recommended structure to specify static power supply
* parameters
- * @cal_charge: calculate charge level.
*/
struct gab_platform_data {
struct power_supply_info battery_info;
- int (*cal_charge)(long value);
};
#endif /* GENERIC_ADC_BATTERY_H */
--
2.39.2
Drop support for configuring IRQ jitter delay by using big
enough fixed value.
Reviewed-by: Linus Walleij <[email protected]>
Signed-off-by: Sebastian Reichel <[email protected]>
---
drivers/power/supply/generic-adc-battery.c | 13 ++++---------
include/linux/power/generic-adc-battery.h | 3 ---
2 files changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/power/supply/generic-adc-battery.c b/drivers/power/supply/generic-adc-battery.c
index 535972a332b3..e20894460d7f 100644
--- a/drivers/power/supply/generic-adc-battery.c
+++ b/drivers/power/supply/generic-adc-battery.c
@@ -227,12 +227,10 @@ static void gab_work(struct work_struct *work)
static irqreturn_t gab_charged(int irq, void *dev_id)
{
struct gab *adc_bat = dev_id;
- struct gab_platform_data *pdata = adc_bat->pdata;
- int delay;
- delay = pdata->jitter_delay ? pdata->jitter_delay : JITTER_DEFAULT;
schedule_delayed_work(&adc_bat->bat_work,
- msecs_to_jiffies(delay));
+ msecs_to_jiffies(JITTER_DEFAULT));
+
return IRQ_HANDLED;
}
@@ -358,14 +356,11 @@ static int __maybe_unused gab_suspend(struct device *dev)
static int __maybe_unused gab_resume(struct device *dev)
{
struct gab *adc_bat = dev_get_drvdata(dev);
- struct gab_platform_data *pdata = adc_bat->pdata;
- int delay;
-
- delay = pdata->jitter_delay ? pdata->jitter_delay : JITTER_DEFAULT;
/* Schedule timer to check current status */
schedule_delayed_work(&adc_bat->bat_work,
- msecs_to_jiffies(delay));
+ msecs_to_jiffies(JITTER_DEFAULT));
+
return 0;
}
diff --git a/include/linux/power/generic-adc-battery.h b/include/linux/power/generic-adc-battery.h
index c68cbf34cd34..50eb4bf28286 100644
--- a/include/linux/power/generic-adc-battery.h
+++ b/include/linux/power/generic-adc-battery.h
@@ -11,13 +11,10 @@
* @battery_info: recommended structure to specify static power supply
* parameters
* @cal_charge: calculate charge level.
- * @jitter_delay: delay required after the interrupt to check battery
- * status.Default set is 10ms.
*/
struct gab_platform_data {
struct power_supply_info battery_info;
int (*cal_charge)(long value);
- int jitter_delay;
};
#endif /* GENERIC_ADC_BATTERY_H */
--
2.39.2
Error printing happens automatically for memory allocation problems.
Reviewed-by: Linus Walleij <[email protected]>
Reviewed-by: Matti Vaittinen <[email protected]>
Signed-off-by: Sebastian Reichel <[email protected]>
---
drivers/power/supply/generic-adc-battery.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/power/supply/generic-adc-battery.c b/drivers/power/supply/generic-adc-battery.c
index d07eeb7d46d3..771e5cfc49c3 100644
--- a/drivers/power/supply/generic-adc-battery.c
+++ b/drivers/power/supply/generic-adc-battery.c
@@ -243,10 +243,8 @@ static int gab_probe(struct platform_device *pdev)
bool any = false;
adc_bat = devm_kzalloc(&pdev->dev, sizeof(*adc_bat), GFP_KERNEL);
- if (!adc_bat) {
- dev_err(&pdev->dev, "failed to allocate memory\n");
+ if (!adc_bat)
return -ENOMEM;
- }
psy_cfg.drv_data = adc_bat;
psy_desc = &adc_bat->psy_desc;
--
2.39.2
From: Sebastian Reichel <[email protected]>
Another typical thing to monitor via an ADC line is
the battery temperature.
Signed-off-by: Sebastian Reichel <[email protected]>
---
drivers/power/supply/generic-adc-battery.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/power/supply/generic-adc-battery.c b/drivers/power/supply/generic-adc-battery.c
index 4811e72df8cd..0124d8d51af7 100644
--- a/drivers/power/supply/generic-adc-battery.c
+++ b/drivers/power/supply/generic-adc-battery.c
@@ -30,6 +30,7 @@ enum gab_chan_type {
GAB_VOLTAGE = 0,
GAB_CURRENT,
GAB_POWER,
+ GAB_TEMP,
GAB_MAX_CHAN_TYPE
};
@@ -40,7 +41,8 @@ enum gab_chan_type {
static const char *const gab_chan_name[] = {
[GAB_VOLTAGE] = "voltage",
[GAB_CURRENT] = "current",
- [GAB_POWER] = "power",
+ [GAB_POWER] = "power",
+ [GAB_TEMP] = "temperature",
};
struct gab {
@@ -77,6 +79,7 @@ static const enum power_supply_property gab_dyn_props[] = {
POWER_SUPPLY_PROP_VOLTAGE_NOW,
POWER_SUPPLY_PROP_CURRENT_NOW,
POWER_SUPPLY_PROP_POWER_NOW,
+ POWER_SUPPLY_PROP_TEMP,
};
static bool gab_charge_finished(struct gab *adc_bat)
@@ -115,6 +118,8 @@ static int gab_get_property(struct power_supply *psy,
return read_channel(adc_bat, GAB_CURRENT, &val->intval);
case POWER_SUPPLY_PROP_POWER_NOW:
return read_channel(adc_bat, GAB_POWER, &val->intval);
+ case POWER_SUPPLY_PROP_TEMP:
+ return read_channel(adc_bat, GAB_TEMP, &val->intval);
default:
return -EINVAL;
}
--
2.39.2
jz4740-battery.c and s3c_adc_battery.c have been removed
from the tree and after all of my restructuring the driver
is basically no longer based on them.
Thus update the copyright information and switch to SPDX
license identifier while being at it.
Reviewed-by: Linus Walleij <[email protected]>
Reviewed-by: Matti Vaittinen <[email protected]>
Signed-off-by: Sebastian Reichel <[email protected]>
---
drivers/power/supply/generic-adc-battery.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/power/supply/generic-adc-battery.c b/drivers/power/supply/generic-adc-battery.c
index e11ad43ab968..df1c0a1c6b52 100644
--- a/drivers/power/supply/generic-adc-battery.c
+++ b/drivers/power/supply/generic-adc-battery.c
@@ -1,13 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0
/*
- * Generic battery driver code using IIO
+ * Generic battery driver using IIO
* Copyright (C) 2012, Anish Kumar <[email protected]>
- * based on jz4740-battery.c
- * based on s3c_adc_battery.c
- *
- * This file is subject to the terms and conditions of the GNU General Public
- * License. See the file COPYING in the main directory of this archive for
- * more details.
- *
+ * Copyright (c) 2023, Sebastian Reichel <[email protected]>
*/
#include <linux/interrupt.h>
#include <linux/platform_device.h>
--
2.39.2
Drop mostly useless gab_prop_to_chan() function by directly
supplying the correct enum value to read_channel().
Reviewed-by: Linus Walleij <[email protected]>
Reviewed-by: Matti Vaittinen <[email protected]>
Signed-off-by: Sebastian Reichel <[email protected]>
---
drivers/power/supply/generic-adc-battery.c | 31 ++++------------------
1 file changed, 5 insertions(+), 26 deletions(-)
diff --git a/drivers/power/supply/generic-adc-battery.c b/drivers/power/supply/generic-adc-battery.c
index d4f63d945b2c..4811e72df8cd 100644
--- a/drivers/power/supply/generic-adc-battery.c
+++ b/drivers/power/supply/generic-adc-battery.c
@@ -86,31 +86,12 @@ static bool gab_charge_finished(struct gab *adc_bat)
return gpiod_get_value(adc_bat->charge_finished);
}
-static enum gab_chan_type gab_prop_to_chan(enum power_supply_property psp)
-{
- switch (psp) {
- case POWER_SUPPLY_PROP_POWER_NOW:
- return GAB_POWER;
- case POWER_SUPPLY_PROP_VOLTAGE_NOW:
- return GAB_VOLTAGE;
- case POWER_SUPPLY_PROP_CURRENT_NOW:
- return GAB_CURRENT;
- default:
- WARN_ON(1);
- break;
- }
- return GAB_POWER;
-}
-
-static int read_channel(struct gab *adc_bat, enum power_supply_property psp,
+static int read_channel(struct gab *adc_bat, enum gab_chan_type channel,
int *result)
{
int ret;
- int chan_index;
- chan_index = gab_prop_to_chan(psp);
- ret = iio_read_channel_processed(adc_bat->channel[chan_index],
- result);
+ ret = iio_read_channel_processed(adc_bat->channel[channel], result);
if (ret < 0)
pr_err("read channel error\n");
else
@@ -129,13 +110,11 @@ static int gab_get_property(struct power_supply *psy,
val->intval = adc_bat->status;
return 0;
case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+ return read_channel(adc_bat, GAB_VOLTAGE, &val->intval);
case POWER_SUPPLY_PROP_CURRENT_NOW:
+ return read_channel(adc_bat, GAB_CURRENT, &val->intval);
case POWER_SUPPLY_PROP_POWER_NOW:
- ret = read_channel(adc_bat, psp, &result);
- if (ret < 0)
- goto err;
- val->intval = result;
- break;
+ return read_channel(adc_bat, GAB_POWER, &val->intval);
default:
return -EINVAL;
}
--
2.39.2
Use standard simple-battery API for constant battery
information like min and max voltage. This simplifies
the driver a lot and brings automatic support for DT.
Reviewed-by: Linus Walleij <[email protected]>
Signed-off-by: Sebastian Reichel <[email protected]>
---
drivers/power/supply/generic-adc-battery.c | 64 ++--------------------
include/linux/power/generic-adc-battery.h | 18 ------
2 files changed, 4 insertions(+), 78 deletions(-)
delete mode 100644 include/linux/power/generic-adc-battery.h
diff --git a/drivers/power/supply/generic-adc-battery.c b/drivers/power/supply/generic-adc-battery.c
index 771e5cfc49c3..d4f63d945b2c 100644
--- a/drivers/power/supply/generic-adc-battery.c
+++ b/drivers/power/supply/generic-adc-battery.c
@@ -22,7 +22,6 @@
#include <linux/slab.h>
#include <linux/iio/consumer.h>
#include <linux/iio/types.h>
-#include <linux/power/generic-adc-battery.h>
#include <linux/devm-helpers.h>
#define JITTER_DEFAULT 10 /* hope 10ms is enough */
@@ -48,9 +47,7 @@ struct gab {
struct power_supply *psy;
struct power_supply_desc psy_desc;
struct iio_channel *channel[GAB_MAX_CHAN_TYPE];
- struct gab_platform_data *pdata;
struct delayed_work bat_work;
- int level;
int status;
bool cable_plugged;
struct gpio_desc *charge_finished;
@@ -70,14 +67,6 @@ static void gab_ext_power_changed(struct power_supply *psy)
static const enum power_supply_property gab_props[] = {
POWER_SUPPLY_PROP_STATUS,
- POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
- POWER_SUPPLY_PROP_CHARGE_EMPTY_DESIGN,
- POWER_SUPPLY_PROP_VOLTAGE_NOW,
- POWER_SUPPLY_PROP_CURRENT_NOW,
- POWER_SUPPLY_PROP_TECHNOLOGY,
- POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
- POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
- POWER_SUPPLY_PROP_MODEL_NAME,
};
/*
@@ -97,17 +86,6 @@ static bool gab_charge_finished(struct gab *adc_bat)
return gpiod_get_value(adc_bat->charge_finished);
}
-static int gab_get_status(struct gab *adc_bat)
-{
- struct gab_platform_data *pdata = adc_bat->pdata;
- struct power_supply_info *bat_info;
-
- bat_info = &pdata->battery_info;
- if (adc_bat->level == bat_info->charge_full_design)
- return POWER_SUPPLY_STATUS_FULL;
- return adc_bat->status;
-}
-
static enum gab_chan_type gab_prop_to_chan(enum power_supply_property psp)
{
switch (psp) {
@@ -144,27 +122,12 @@ static int read_channel(struct gab *adc_bat, enum power_supply_property psp,
static int gab_get_property(struct power_supply *psy,
enum power_supply_property psp, union power_supply_propval *val)
{
- struct gab *adc_bat;
- struct gab_platform_data *pdata;
- struct power_supply_info *bat_info;
- int result = 0;
- int ret = 0;
-
- adc_bat = to_generic_bat(psy);
- if (!adc_bat) {
- dev_err(&psy->dev, "no battery infos ?!\n");
- return -EINVAL;
- }
- pdata = adc_bat->pdata;
- bat_info = &pdata->battery_info;
+ struct gab *adc_bat = to_generic_bat(psy);
switch (psp) {
case POWER_SUPPLY_PROP_STATUS:
- val->intval = gab_get_status(adc_bat);
- break;
- case POWER_SUPPLY_PROP_CHARGE_EMPTY_DESIGN:
- val->intval = 0;
- break;
+ val->intval = adc_bat->status;
+ return 0;
case POWER_SUPPLY_PROP_VOLTAGE_NOW:
case POWER_SUPPLY_PROP_CURRENT_NOW:
case POWER_SUPPLY_PROP_POWER_NOW:
@@ -173,26 +136,9 @@ static int gab_get_property(struct power_supply *psy,
goto err;
val->intval = result;
break;
- case POWER_SUPPLY_PROP_TECHNOLOGY:
- val->intval = bat_info->technology;
- break;
- case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
- val->intval = bat_info->voltage_min_design;
- break;
- case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
- val->intval = bat_info->voltage_max_design;
- break;
- case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
- val->intval = bat_info->charge_full_design;
- break;
- case POWER_SUPPLY_PROP_MODEL_NAME:
- val->strval = bat_info->name;
- break;
default:
return -EINVAL;
}
-err:
- return ret;
}
static void gab_work(struct work_struct *work)
@@ -235,7 +181,6 @@ static int gab_probe(struct platform_device *pdev)
struct gab *adc_bat;
struct power_supply_desc *psy_desc;
struct power_supply_config psy_cfg = {};
- struct gab_platform_data *pdata = pdev->dev.platform_data;
enum power_supply_property *properties;
int ret = 0;
int chan;
@@ -248,7 +193,7 @@ static int gab_probe(struct platform_device *pdev)
psy_cfg.drv_data = adc_bat;
psy_desc = &adc_bat->psy_desc;
- psy_desc->name = pdata->battery_info.name;
+ psy_desc->name = dev_name(&pdev->dev);
/* bootup default values for the battery */
adc_bat->cable_plugged = false;
@@ -256,7 +201,6 @@ static int gab_probe(struct platform_device *pdev)
psy_desc->type = POWER_SUPPLY_TYPE_BATTERY;
psy_desc->get_property = gab_get_property;
psy_desc->external_power_changed = gab_ext_power_changed;
- adc_bat->pdata = pdata;
/*
* copying the static properties and allocating extra memory for holding
diff --git a/include/linux/power/generic-adc-battery.h b/include/linux/power/generic-adc-battery.h
deleted file mode 100644
index 54434e4304d3..000000000000
--- a/include/linux/power/generic-adc-battery.h
+++ /dev/null
@@ -1,18 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * Copyright (C) 2012, Anish Kumar <[email protected]>
- */
-
-#ifndef GENERIC_ADC_BATTERY_H
-#define GENERIC_ADC_BATTERY_H
-
-/**
- * struct gab_platform_data - platform_data for generic adc iio battery driver.
- * @battery_info: recommended structure to specify static power supply
- * parameters
- */
-struct gab_platform_data {
- struct power_supply_info battery_info;
-};
-
-#endif /* GENERIC_ADC_BATTERY_H */
--
2.39.2
On 3/15/23 00:55, Sebastian Reichel wrote:
> Add binding for a battery that is only monitored via ADC
> channels and simple status GPIOs.
>
> Signed-off-by: Sebastian Reichel <[email protected]>
> ---
> .../bindings/power/supply/adc-battery.yaml | 70 +++++++++++++++++++
> 1 file changed, 70 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/power/supply/adc-battery.yaml
>
> diff --git a/Documentation/devicetree/bindings/power/supply/adc-battery.yaml b/Documentation/devicetree/bindings/power/supply/adc-battery.yaml
> new file mode 100644
> index 000000000000..ed9702caedff
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/adc-battery.yaml
> @@ -0,0 +1,70 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/supply/adc-battery.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ADC battery
> +
> +maintainers:
> + - Sebastian Reichel <[email protected]>
> +
> +description:
> + Basic battery capacity meter, which only reports basic battery data
> + via ADC channels and optionally indicate that the battery is full by
> + polling a GPIO line.
> +
> + The voltage is expected to be measured between the battery terminals
> + and mandatory. The optional current/power channel is expected to
> + monitor the current/power flowing out of the battery. Last but not
> + least the temperature channel is supposed to measure the battery
> + temperature.
This looks very good to me. Even I fell under the illusion I know what
these bindings mean and how the hardware looks like ;)
I am terrible with the bindings syntax so this is not worth much but:
Reviewed-by: Matti Vaittinen <[email protected]>
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 14/03/2023 23:55, Sebastian Reichel wrote:
> Add binding for a battery that is only monitored via ADC
> channels and simple status GPIOs.
>
> Signed-off-by: Sebastian Reichel <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On 3/15/23 00:55, Sebastian Reichel wrote:
> Add support for automatically exposing data from the
> simple-battery firmware node with a single configuration
> option in the power-supply device.
>
> Signed-off-by: Sebastian Reichel <[email protected]>
> ---
> drivers/power/supply/power_supply_core.c | 173 +++++++++++++++++++---
> drivers/power/supply/power_supply_sysfs.c | 15 ++
> include/linux/power_supply.h | 8 +
> 3 files changed, 178 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index f3d7c1da299f..842c27de4fac 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -388,7 +388,7 @@ static int __power_supply_get_supplier_property(struct device *dev, void *_data)
> struct psy_get_supplier_prop_data *data = _data;
>
> if (__power_supply_is_supplied_by(epsy, data->psy))
> - if (!epsy->desc->get_property(epsy, data->psp, data->val))
> + if (!power_supply_get_property(epsy, data->psp, data->val))
> return 1; /* Success */
>
> return 0; /* Continue iterating */
> @@ -832,6 +832,133 @@ void power_supply_put_battery_info(struct power_supply *psy,
> }
> EXPORT_SYMBOL_GPL(power_supply_put_battery_info);
>
> +const enum power_supply_property power_supply_battery_info_properties[] = {
> + POWER_SUPPLY_PROP_TECHNOLOGY,
> + POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
> + POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
> + POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
> + POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
> + POWER_SUPPLY_PROP_PRECHARGE_CURRENT,
> + POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT,
> + POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
> + POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
> + POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN,
> + POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX,
> + POWER_SUPPLY_PROP_TEMP_ALERT_MIN,
> + POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
> + POWER_SUPPLY_PROP_TEMP_MIN,
> + POWER_SUPPLY_PROP_TEMP_MAX,
> +};
> +EXPORT_SYMBOL_GPL(power_supply_battery_info_properties);
> +
> +const size_t power_supply_battery_info_properties_size = ARRAY_SIZE(power_supply_battery_info_properties);
> +EXPORT_SYMBOL_GPL(power_supply_battery_info_properties_size);
> +
> +bool power_supply_battery_info_has_prop(struct power_supply_battery_info *info,
> + enum power_supply_property psp)
> +{
> + if (!info)
> + return false;
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_TECHNOLOGY:
> + return info->technology != POWER_SUPPLY_TECHNOLOGY_UNKNOWN;
> + case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN:
> + return info->energy_full_design_uwh >= 0;
> + case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> + return info->charge_full_design_uah >= 0;
> + case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
> + return info->voltage_min_design_uv >= 0;
> + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
> + return info->voltage_max_design_uv >= 0;
> + case POWER_SUPPLY_PROP_PRECHARGE_CURRENT:
> + return info->precharge_current_ua >= 0;
> + case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT:
> + return info->charge_term_current_ua >= 0;
> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
> + return info->constant_charge_current_max_ua >= 0;
> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
> + return info->constant_charge_voltage_max_uv >= 0;
> + case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN:
> + return info->temp_ambient_alert_min > INT_MIN;
> + case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX:
> + return info->temp_ambient_alert_max < INT_MAX;
> + case POWER_SUPPLY_PROP_TEMP_ALERT_MIN:
> + return info->temp_alert_min > INT_MIN;
> + case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
> + return info->temp_alert_max < INT_MAX;
> + case POWER_SUPPLY_PROP_TEMP_MIN:
> + return info->temp_min > INT_MIN;
> + case POWER_SUPPLY_PROP_TEMP_MAX:
> + return info->temp_max < INT_MAX;
> + default:
> + return false;
> + }
> +}
> +EXPORT_SYMBOL_GPL(power_supply_battery_info_has_prop);
> +
> +int power_supply_battery_info_get_prop(struct power_supply_battery_info *info,
> + enum power_supply_property psp,
> + union power_supply_propval *val)
> +{
> + if (!info)
> + return -EINVAL;
> +
> + if (!power_supply_battery_info_has_prop(info, psp))
> + return -EINVAL;
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_TECHNOLOGY:
> + val->intval = info->technology;
> + return 0;
> + case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN:
> + val->intval = info->energy_full_design_uwh;
> + return 0;
> + case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> + val->intval = info->charge_full_design_uah;
> + return 0;
> + case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
> + val->intval = info->voltage_min_design_uv;
> + return 0;
> + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
> + val->intval = info->voltage_max_design_uv;
> + return 0;
> + case POWER_SUPPLY_PROP_PRECHARGE_CURRENT:
> + val->intval = info->precharge_current_ua;
> + return 0;
> + case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT:
> + val->intval = info->charge_term_current_ua;
> + return 0;
> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
> + val->intval = info->constant_charge_current_max_ua;
> + return 0;
> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
> + val->intval = info->constant_charge_voltage_max_uv;
> + return 0;
> + case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN:
> + val->intval = info->temp_ambient_alert_min;
> + return 0;
> + case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX:
> + val->intval = info->temp_ambient_alert_max;
> + return 0;
> + case POWER_SUPPLY_PROP_TEMP_ALERT_MIN:
> + val->intval = info->temp_alert_min;
> + return 0;
> + case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
> + val->intval = info->temp_alert_max;
> + return 0;
> + case POWER_SUPPLY_PROP_TEMP_MIN:
> + val->intval = info->temp_min;
> + return 0;
> + case POWER_SUPPLY_PROP_TEMP_MAX:
> + val->intval = info->temp_max;
> + return 0;
> + default:
> + return -EINVAL;
> + }
> +}
> +EXPORT_SYMBOL_GPL(power_supply_battery_info_get_prop);
> +
> /**
> * power_supply_temp2resist_simple() - find the battery internal resistance
> * percent from temperature
> @@ -1046,6 +1173,22 @@ 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 found = false;
> + int i;
> +
> + for (i = 0; i < psy_desc->num_properties; i++) {
> + if (psy_desc->properties[i] == psp) {
> + found = true;
> + break;
> + }
> + }
> +
> + return found;
> +}
> +
> int power_supply_get_property(struct power_supply *psy,
> enum power_supply_property psp,
> union power_supply_propval *val)
> @@ -1056,7 +1199,12 @@ int power_supply_get_property(struct power_supply *psy,
> return -ENODEV;
> }
>
> - return psy->desc->get_property(psy, psp, val);
> + if (psy_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);
> + else
> + return -EINVAL;
> }
> EXPORT_SYMBOL_GPL(power_supply_get_property);
>
> @@ -1117,22 +1265,6 @@ void power_supply_unreg_notifier(struct notifier_block *nb)
> }
> EXPORT_SYMBOL_GPL(power_supply_unreg_notifier);
>
> -static bool psy_has_property(const struct power_supply_desc *psy_desc,
> - enum power_supply_property psp)
> -{
> - bool found = false;
> - int i;
> -
> - for (i = 0; i < psy_desc->num_properties; i++) {
> - if (psy_desc->properties[i] == psp) {
> - found = true;
> - break;
> - }
> - }
> -
> - return found;
> -}
> -
I do really like everything in this patch up to this point :) Core
providing properties to the user based on the battery-info seems great
to me.
> #ifdef CONFIG_THERMAL
> static int power_supply_read_temp(struct thermal_zone_device *tzd,
> int *temp)
> @@ -1255,6 +1387,11 @@ __power_supply_register(struct device *parent,
> goto check_supplies_failed;
> }
>
> + /* psy->battery_info is optional */
> + rc = power_supply_get_battery_info(psy, &psy->battery_info);
> + if (rc && rc != -ENODEV)
> + goto check_supplies_failed;
> +
This is what rubs me in a slightly wrong way - but again, you probably
know better than I what's the direction things are heading so please
ignore me if I am talking nonsense :)
Anyways, I think the battery information may be relevant to the driver
which is registering the power-supply. It may be there is a fuel-gauge
which needs to know the capacity and OCV tables etc. Or some other
thingy. And - I may be wrong - but I have a feeling it might be
something that should be known prior registering the power-supply.
So, in my head it should be the driver which is getting the information
about the battery (whether it is in the DT node or coded in some tables
and fetched by battery type) - using helpers provided by core.
I further think it should be the driver who can pass the battery
information to core at registration - core may 'fall-back' finding
information itself if driver did not provide it.
So, I think the core should not unconditionally populate the
battery-info here but it should first check if the driver had it already
filled.
Well, as I said, I recognize I may not (do not) know all the dirty
details and I do trust you to evaluate if what I wrote here makes any
sense :) All in all, I think this auto-exposure is great.
Please, bear with me if what I wrote above does not make sense to you
and just assume I don't see the big picture :)
> spin_lock_init(&psy->changed_lock);
> rc = device_add(dev);
> if (rc)
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index c228205e0953..5842dfe5dfb7 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -380,6 +380,9 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
> }
> }
>
> + if (power_supply_battery_info_has_prop(psy->battery_info, attrno))
> + return mode;
> +
> return 0;
> }
>
> @@ -461,6 +464,8 @@ static int add_prop_uevent(const struct device *dev, struct kobj_uevent_env *env
> int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
> {
> const struct power_supply *psy = dev_get_drvdata(dev);
> + const enum power_supply_property *battery_props =
> + power_supply_battery_info_properties;
> int ret = 0, j;
> char *prop_buf;
>
> @@ -488,6 +493,16 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
> goto out;
> }
>
> + for (j = 0; j < power_supply_battery_info_properties_size; j++) {
> + if (!power_supply_battery_info_has_prop(psy->battery_info,
> + battery_props[j]))
> + continue;
Hmm. I just noticed that there can probably be same properties in the
psy->desc->properties and in the battery-info. I didn't cascade deep
into the code so I can't say if it is a problem to add duplicates?
So, if this is safe, and if what I wrote above is not something you want
to consider:
Reviewed-by: Matti Vaittinen <[email protected]>
> + ret = add_prop_uevent(dev, env, battery_props[j],
> + prop_buf);
> + if (ret)
> + goto out;
> + }
> +
> out:
> free_page((unsigned long)prop_buf);
>
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index aa2c4a7c4826..a427f13c757f 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -301,6 +301,7 @@ struct power_supply {
> bool initialized;
> bool removing;
> atomic_t use_cnt;
> + struct power_supply_battery_info *battery_info;
> #ifdef CONFIG_THERMAL
> struct thermal_zone_device *tzd;
> struct thermal_cooling_device *tcd;
> @@ -791,10 +792,17 @@ devm_power_supply_get_by_phandle(struct device *dev, const char *property)
> { return NULL; }
> #endif /* CONFIG_OF */
>
> +extern const enum power_supply_property power_supply_battery_info_properties[];
> +extern const size_t power_supply_battery_info_properties_size;
> extern int power_supply_get_battery_info(struct power_supply *psy,
> struct power_supply_battery_info **info_out);
> extern void power_supply_put_battery_info(struct power_supply *psy,
> struct power_supply_battery_info *info);
> +extern bool power_supply_battery_info_has_prop(struct power_supply_battery_info *info,
> + enum power_supply_property psp);
> +extern int power_supply_battery_info_get_prop(struct power_supply_battery_info *info,
> + enum power_supply_property psp,
> + union power_supply_propval *val);
> extern int power_supply_ocv2cap_simple(struct power_supply_battery_ocv_table *table,
> int table_len, int ocv);
> extern struct power_supply_battery_ocv_table *
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 3/15/23 00:55, Sebastian Reichel wrote:
> From: Sebastian Reichel <[email protected]>
>
> Another typical thing to monitor via an ADC line is
> the battery temperature.
>
> Signed-off-by: Sebastian Reichel <[email protected]>
Reviewed-by: Matti Vaittinen <[email protected]>
> ---
> drivers/power/supply/generic-adc-battery.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/power/supply/generic-adc-battery.c b/drivers/power/supply/generic-adc-battery.c
> index 4811e72df8cd..0124d8d51af7 100644
> --- a/drivers/power/supply/generic-adc-battery.c
> +++ b/drivers/power/supply/generic-adc-battery.c
> @@ -30,6 +30,7 @@ enum gab_chan_type {
> GAB_VOLTAGE = 0,
> GAB_CURRENT,
> GAB_POWER,
> + GAB_TEMP,
> GAB_MAX_CHAN_TYPE
> };
>
> @@ -40,7 +41,8 @@ enum gab_chan_type {
> static const char *const gab_chan_name[] = {
> [GAB_VOLTAGE] = "voltage",
> [GAB_CURRENT] = "current",
> - [GAB_POWER] = "power",
> + [GAB_POWER] = "power",
> + [GAB_TEMP] = "temperature",
> };
>
> struct gab {
> @@ -77,6 +79,7 @@ static const enum power_supply_property gab_dyn_props[] = {
> POWER_SUPPLY_PROP_VOLTAGE_NOW,
> POWER_SUPPLY_PROP_CURRENT_NOW,
> POWER_SUPPLY_PROP_POWER_NOW,
> + POWER_SUPPLY_PROP_TEMP,
> };
>
> static bool gab_charge_finished(struct gab *adc_bat)
> @@ -115,6 +118,8 @@ static int gab_get_property(struct power_supply *psy,
> return read_channel(adc_bat, GAB_CURRENT, &val->intval);
> case POWER_SUPPLY_PROP_POWER_NOW:
> return read_channel(adc_bat, GAB_POWER, &val->intval);
> + case POWER_SUPPLY_PROP_TEMP:
> + return read_channel(adc_bat, GAB_TEMP, &val->intval);
> default:
> return -EINVAL;
> }
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On Tue, Mar 14, 2023 at 11:55 PM Sebastian Reichel <[email protected]> wrote:
> Add binding for a battery that is only monitored via ADC
> channels and simple status GPIOs.
>
> Signed-off-by: Sebastian Reichel <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Tue, Mar 14, 2023 at 11:55 PM Sebastian Reichel <[email protected]> wrote:
> From: Sebastian Reichel <[email protected]>
>
> Another typical thing to monitor via an ADC line is
> the battery temperature.
>
> Signed-off-by: Sebastian Reichel <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
> static bool gab_charge_finished(struct gab *adc_bat)
> @@ -115,6 +118,8 @@ static int gab_get_property(struct power_supply *psy,
> return read_channel(adc_bat, GAB_CURRENT, &val->intval);
> case POWER_SUPPLY_PROP_POWER_NOW:
> return read_channel(adc_bat, GAB_POWER, &val->intval);
> + case POWER_SUPPLY_PROP_TEMP:
> + return read_channel(adc_bat, GAB_TEMP, &val->intval);
Hm. I wonder if these should rather all use read_channel_processed()?
The difference is that you will then support ADCs with internal scaling
which is beneficial. Most of the time it doesn't matter.
Yours,
Linus Walleij
Hi Linus,
On Wed, Mar 15, 2023 at 09:04:15AM +0100, Linus Walleij wrote:
> On Tue, Mar 14, 2023 at 11:55 PM Sebastian Reichel <[email protected]> wrote:
> > From: Sebastian Reichel <[email protected]>
> > Another typical thing to monitor via an ADC line is
> > the battery temperature.
> >
> > Signed-off-by: Sebastian Reichel <[email protected]>
>
> Reviewed-by: Linus Walleij <[email protected]>
>
> > static bool gab_charge_finished(struct gab *adc_bat)
> > @@ -115,6 +118,8 @@ static int gab_get_property(struct power_supply *psy,
> > return read_channel(adc_bat, GAB_CURRENT, &val->intval);
> > case POWER_SUPPLY_PROP_POWER_NOW:
> > return read_channel(adc_bat, GAB_POWER, &val->intval);
> > + case POWER_SUPPLY_PROP_TEMP:
> > + return read_channel(adc_bat, GAB_TEMP, &val->intval);
>
> Hm. I wonder if these should rather all use read_channel_processed()?
>
> The difference is that you will then support ADCs with internal scaling
> which is beneficial. Most of the time it doesn't matter.
read_channel is a local helper, the driver uses the processed
variant of iio_read_channel:
static int read_channel(struct gab *adc_bat, enum gab_chan_type channel,
int *result)
{
int ret;
ret = iio_read_channel_processed(adc_bat->channel[channel], result);
if (ret < 0)
pr_err("read channel error\n");
else
*result *= 1000;
return ret;
}
-- Sebastian
Hi,
On Wed, Mar 15, 2023 at 09:01:50AM +0200, Matti Vaittinen wrote:
> On 3/15/23 00:55, Sebastian Reichel wrote:
> [...]
> > #ifdef CONFIG_THERMAL
> > static int power_supply_read_temp(struct thermal_zone_device *tzd,
> > int *temp)
> > @@ -1255,6 +1387,11 @@ __power_supply_register(struct device *parent,
> > goto check_supplies_failed;
> > }
> > + /* psy->battery_info is optional */
I forgot to add a POWER_SUPPLY_TYPE_BATTERY limitation when removing
the opt-in method. Will be added in the next revision.
> > + rc = power_supply_get_battery_info(psy, &psy->battery_info);
> > + if (rc && rc != -ENODEV)
> > + goto check_supplies_failed;
> > +
>
> This is what rubs me in a slightly wrong way - but again, you probably know
> better than I what's the direction things are heading so please ignore me if
> I am talking nonsense :)
>
> Anyways, I think the battery information may be relevant to the driver which
> is registering the power-supply. It may be there is a fuel-gauge which needs
> to know the capacity and OCV tables etc. Or some other thingy. And - I may
> be wrong - but I have a feeling it might be something that should be known
> prior registering the power-supply.
You can still do that, just like before. It's a bit inefficient,
since the battery data is allocated twice, but the driver probe
routine is not a hot path.
> So, in my head it should be the driver which is getting the information
> about the battery (whether it is in the DT node or coded in some tables and
> fetched by battery type) - using helpers provided by core.
>
> I further think it should be the driver who can pass the battery information
> to core at registration - core may 'fall-back' finding information itself if
> driver did not provide it.
This implements the fallback route.
> So, I think the core should not unconditionally populate the battery-info
> here but it should first check if the driver had it already filled.
Not until there is a user (i.e. a driver using that feature). FWIW
it's quite easy to implement once it is needed. Just adding a field
in power_supply_config and taking it over here is enough, no other
code changes are required.
The alternative is adding some kind of probe/remove callback for the
power_supply device itself to properly initialize the device. That
would also be useful to have a sensible place for e.g. shutting of
chargers when the device is removed. Anyways it's a bit out of scope
for this patchset :)
> Well, as I said, I recognize I may not (do not) know all the dirty details
> and I do trust you to evaluate if what I wrote here makes any sense :) All
> in all, I think this auto-exposure is great.
>
> Please, bear with me if what I wrote above does not make sense to you and
> just assume I don't see the big picture :)
Right now the following battery drivers use power_supply_get_battery_info():
* cw2015_battery
* bq27xxx_battery
* axp20x_battery
* ug3105_battery
* ingenic-battery
* sc27xx_fuel_gauge
* (generic-adc-battery)
All of them call it after the power-supply device has been
registered. Thus the way to go for them is removing the second call
to power_supply_get_battery_info() and instead use the battery-info
acquired by the core. I think that work deserves its own series.
For chargers the situation is different (they usually want the data
before registration), but they should not expose the battery data
anyways. Also ideally chargers get the information from the battery
power-supply device, which might supply the data from fuel-gauge
registers (or fallback to battery-info after this series).
> > spin_lock_init(&psy->changed_lock);
> > rc = device_add(dev);
> > if (rc)
> > diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> > index c228205e0953..5842dfe5dfb7 100644
> > --- a/drivers/power/supply/power_supply_sysfs.c
> > +++ b/drivers/power/supply/power_supply_sysfs.c
> > @@ -380,6 +380,9 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
> > }
> > }
> > + if (power_supply_battery_info_has_prop(psy->battery_info, attrno))
> > + return mode;
> > +
> > return 0;
> > }
> > @@ -461,6 +464,8 @@ static int add_prop_uevent(const struct device *dev, struct kobj_uevent_env *env
> > int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
> > {
> > const struct power_supply *psy = dev_get_drvdata(dev);
> > + const enum power_supply_property *battery_props =
> > + power_supply_battery_info_properties;
> > int ret = 0, j;
> > char *prop_buf;
> > @@ -488,6 +493,16 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
> > goto out;
> > }
> > + for (j = 0; j < power_supply_battery_info_properties_size; j++) {
> > + if (!power_supply_battery_info_has_prop(psy->battery_info,
> > + battery_props[j]))
> > + continue;
>
> Hmm. I just noticed that there can probably be same properties in the
> psy->desc->properties and in the battery-info.
That's intended, so that battery drivers can implement their own
behaviour for the properties.
> I didn't cascade deep into the code so I can't say if it is a
> problem to add duplicates?
It does not break anything (we used to have this for the TYPE
property in a driver), but confuses userspace. I will fix the
duplication in uevents and send a new version.
> So, if this is safe, and if what I wrote above is not something
> you want to consider:
>
> Reviewed-by: Matti Vaittinen <[email protected]>
Due to the changes I will not take this over in v3. Just to make
sure - is it correct, that you do not want your R-b tag for the
following two patches?
[05/12] power: supply: generic-adc-battery: drop jitter delay support
[08/12] power: supply: generic-adc-battery: use simple-battery API
> [...]
Thanks for your reviews,
-- Sebastian
On 3/16/23 02:41, Sebastian Reichel wrote:
> Hi,
>
> On Wed, Mar 15, 2023 at 09:01:50AM +0200, Matti Vaittinen wrote:
>> On 3/15/23 00:55, Sebastian Reichel wrote:
>> [...]
>>> #ifdef CONFIG_THERMAL
>>> static int power_supply_read_temp(struct thermal_zone_device *tzd,
>>> int *temp)
>>> @@ -1255,6 +1387,11 @@ __power_supply_register(struct device *parent,
>>> goto check_supplies_failed;
>>> }
>>> + /* psy->battery_info is optional */
>
> I forgot to add a POWER_SUPPLY_TYPE_BATTERY limitation when removing
> the opt-in method. Will be added in the next revision.
>
>>> + rc = power_supply_get_battery_info(psy, &psy->battery_info);
>>> + if (rc && rc != -ENODEV)
>>> + goto check_supplies_failed;
>>> +
>>
>> This is what rubs me in a slightly wrong way - but again, you probably know
>> better than I what's the direction things are heading so please ignore me if
>> I am talking nonsense :)
>>
>> Anyways, I think the battery information may be relevant to the driver which
>> is registering the power-supply. It may be there is a fuel-gauge which needs
>> to know the capacity and OCV tables etc. Or some other thingy. And - I may
>> be wrong - but I have a feeling it might be something that should be known
>> prior registering the power-supply.
>
> You can still do that, just like before.
This is out of scope of your series, but as far as I remember the
"battery info getter" power_supply_get_battery_info() provided by the
core required the struct power_supply - which, I believe, should not be
used prior supply registration.
(I think I did drop this requirement and added a getter which allowed
just passing the device (or fwnode) in a simple-gauge RFC series I sent
couple of years ago - but as I said, this is out of the scope)
> It's a bit inefficient,
> since the battery data is allocated twice, but the driver probe
> routine is not a hot path.
This is true. OTOH, people are constantly struggling to push down the
start-up times so maybe we should avoid feeling too comfortable with
probes being slow. Still, I am not opposed to this series!
>> So, in my head it should be the driver which is getting the information
>> about the battery (whether it is in the DT node or coded in some tables and
>> fetched by battery type) - using helpers provided by core.
>>
>> I further think it should be the driver who can pass the battery information
>> to core at registration - core may 'fall-back' finding information itself if
>> driver did not provide it.
>
> This implements the fallback route.
>
>> So, I think the core should not unconditionally populate the battery-info
>> here but it should first check if the driver had it already filled.
>
> Not until there is a user (i.e. a driver using that feature). FWIW
> it's quite easy to implement once it is needed. Just adding a field
> in power_supply_config and taking it over here is enough, no other
> code changes are required.
Fair enough. You are probably right in that things should not be
complicated if there is no need.
> The alternative is adding some kind of probe/remove callback for the
> power_supply device itself to properly initialize the device. That
> would also be useful to have a sensible place for e.g. shutting of
> chargers when the device is removed. Anyways it's a bit out of scope
> for this patchset :)
>
>> Well, as I said, I recognize I may not (do not) know all the dirty details
>> and I do trust you to evaluate if what I wrote here makes any sense :) All
>> in all, I think this auto-exposure is great.
>>
>> Please, bear with me if what I wrote above does not make sense to you and
>> just assume I don't see the big picture :)
>
> Right now the following battery drivers use power_supply_get_battery_info():
>
> * cw2015_battery
> * bq27xxx_battery
> * axp20x_battery
> * ug3105_battery
> * ingenic-battery
> * sc27xx_fuel_gauge
> * (generic-adc-battery)
>
> All of them call it after the power-supply device has been
> registered. Thus the way to go for them is removing the second call
> to power_supply_get_battery_info() and instead use the battery-info
> acquired by the core. I think that work deserves its own series.
I agree.
> For chargers the situation is different (they usually want the data
> before registration), but they should not expose the battery data
> anyways.
I probably should go back studying how the power-supply class works
before continuing this discussion :)
So, is it so that when a single IC contains both the charger logic and
battery monitoring - then the driver is expected to create two
power_supply devices. One for the battery and the other for the charger?
I assume both of these pover_supply devices will then find the same
battery_info - which means that one of the devices (probably the
charger) should not auto-expose properties(?)
Well, as I said I should go study things better before continuing - but
as I have limited time for studying this now I'll just ask if there is a
danger we auto-expose battery details from existing drivers via two
devices? And as I did no study I will just accept what ever answer you
give and trust you to know this better ^_^;
> Also ideally chargers get the information from the battery
> power-supply device, which might supply the data from fuel-gauge
> registers (or fallback to battery-info after this series).
>
>>> spin_lock_init(&psy->changed_lock);
>>> rc = device_add(dev);
>>> if (rc)
>>> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
>>> index c228205e0953..5842dfe5dfb7 100644
>>> --- a/drivers/power/supply/power_supply_sysfs.c
>>> +++ b/drivers/power/supply/power_supply_sysfs.c
>>> @@ -380,6 +380,9 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
>>> }
>>> }
>>> + if (power_supply_battery_info_has_prop(psy->battery_info, attrno))
>>> + return mode;
>>> +
>>> return 0;
>>> }
>>> @@ -461,6 +464,8 @@ static int add_prop_uevent(const struct device *dev, struct kobj_uevent_env *env
>>> int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
>>> {
>>> const struct power_supply *psy = dev_get_drvdata(dev);
>>> + const enum power_supply_property *battery_props =
>>> + power_supply_battery_info_properties;
>>> int ret = 0, j;
>>> char *prop_buf;
>>> @@ -488,6 +493,16 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
>>> goto out;
>>> }
>>> + for (j = 0; j < power_supply_battery_info_properties_size; j++) {
>>> + if (!power_supply_battery_info_has_prop(psy->battery_info,
>>> + battery_props[j]))
>>> + continue;
>>
>> Hmm. I just noticed that there can probably be same properties in the
>> psy->desc->properties and in the battery-info.
>
> That's intended, so that battery drivers can implement their own
> behaviour for the properties.
Yes, I thought so. But this is why I asked if doubling the uevents below
was a problem.
>> I didn't cascade deep into the code so I can't say if it is a
>> problem to add duplicates?
>
> It does not break anything (we used to have this for the TYPE
> property in a driver), but confuses userspace. I will fix the
> duplication in uevents and send a new version.
>
>> So, if this is safe, and if what I wrote above is not something
>> you want to consider:
>>
>> Reviewed-by: Matti Vaittinen <[email protected]>
>
> Due to the changes I will not take this over in v3. Just to make
> sure - is it correct, that you do not want your R-b tag for the
> following two patches?
>
> [05/12] power: supply: generic-adc-battery: drop jitter delay support
I didn't feel technically capable of reviewing the above delay patch as
I don't know what kind of delays are typically needed - or if they need
to be configurable - or what is the purpose of this delay (some
stabilization period after charging?)
So, it's not that your patch had something I didn't agree with - I was
just not feeling I understand the consequences of the changes. Purely
from coding perspective it looked good to me :)
> [08/12] power: supply: generic-adc-battery: use simple-battery API
This one did look good to me but as it was pretty trivial one I didn't
think my review made much of a difference :) I can reply with my tag on
that one though as I did review what there was to review.
>
>> [...]
>
> Thanks for your reviews,
Thanks to you! You are the one making things better here, I am just
treating this as an opportunity to learn ;)
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 3/16/23 09:10, Matti Vaittinen wrote:
> On 3/16/23 02:41, Sebastian Reichel wrote:
>> Hi,
>> [08/12] power: supply: generic-adc-battery: use simple-battery API
>
> This one did look good to me but as it was pretty trivial one I didn't
> think my review made much of a difference :) I can reply with my tag on
> that one though as I did review what there was to review.
Sorry! I mixed this patch with another one. This indeed did have some
changes - I must've accidentally skipped this one. Will check this after
eating my breakfast :)
>
>>
>>> [...]
>>
>> Thanks for your reviews,
>
> Thanks to you! You are the one making things better here, I am just
> treating this as an opportunity to learn ;)
>
> Yours,
> -- Matti
>
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 3/15/23 00:55, Sebastian Reichel wrote:
> Use standard simple-battery API for constant battery
> information like min and max voltage. This simplifies
> the driver a lot and brings automatic support for DT.
>
> Reviewed-by: Linus Walleij <[email protected]>
> Signed-off-by: Sebastian Reichel <[email protected]>
> ---
> drivers/power/supply/generic-adc-battery.c | 64 ++--------------------
> include/linux/power/generic-adc-battery.h | 18 ------
> 2 files changed, 4 insertions(+), 78 deletions(-)
> delete mode 100644 include/linux/power/generic-adc-battery.h
>
> diff --git a/drivers/power/supply/generic-adc-battery.c b/drivers/power/supply/generic-adc-battery.c
> index 771e5cfc49c3..d4f63d945b2c 100644
> --- a/drivers/power/supply/generic-adc-battery.c
> +++ b/drivers/power/supply/generic-adc-battery.c
> @@ -22,7 +22,6 @@
> #include <linux/slab.h>
> #include <linux/iio/consumer.h>
> #include <linux/iio/types.h>
> -#include <linux/power/generic-adc-battery.h>
> #include <linux/devm-helpers.h>
>
> #define JITTER_DEFAULT 10 /* hope 10ms is enough */
> @@ -48,9 +47,7 @@ struct gab {
> struct power_supply *psy;
> struct power_supply_desc psy_desc;
> struct iio_channel *channel[GAB_MAX_CHAN_TYPE];
> - struct gab_platform_data *pdata;
> struct delayed_work bat_work;
> - int level;
> int status;
> bool cable_plugged;
> struct gpio_desc *charge_finished;
> @@ -70,14 +67,6 @@ static void gab_ext_power_changed(struct power_supply *psy)
>
> static const enum power_supply_property gab_props[] = {
> POWER_SUPPLY_PROP_STATUS,
> - POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
> - POWER_SUPPLY_PROP_CHARGE_EMPTY_DESIGN,
> - POWER_SUPPLY_PROP_VOLTAGE_NOW,
> - POWER_SUPPLY_PROP_CURRENT_NOW,
> - POWER_SUPPLY_PROP_TECHNOLOGY,
> - POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
> - POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
> - POWER_SUPPLY_PROP_MODEL_NAME,
> };
>
> /*
> @@ -97,17 +86,6 @@ static bool gab_charge_finished(struct gab *adc_bat)
> return gpiod_get_value(adc_bat->charge_finished);
> }
>
> -static int gab_get_status(struct gab *adc_bat)
> -{
> - struct gab_platform_data *pdata = adc_bat->pdata;
> - struct power_supply_info *bat_info;
> -
> - bat_info = &pdata->battery_info;
> - if (adc_bat->level == bat_info->charge_full_design)
> - return POWER_SUPPLY_STATUS_FULL;
Not sure if this is intentional but I don't see the
POWER_SUPPLY_STATUS_FULL being reported after applying your series. If
this is intended, maybe it could be mentioned in commit log?
Other than that - this really cleans up the driver in a nice way!
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
Hi,
On Thu, Mar 16, 2023 at 09:38:21AM +0200, Matti Vaittinen wrote:
> On 3/15/23 00:55, Sebastian Reichel wrote:
> > Use standard simple-battery API for constant battery
> > information like min and max voltage. This simplifies
> > the driver a lot and brings automatic support for DT.
> >
> > Reviewed-by: Linus Walleij <[email protected]>
> > Signed-off-by: Sebastian Reichel <[email protected]>
> > ---
> > drivers/power/supply/generic-adc-battery.c | 64 ++--------------------
> > include/linux/power/generic-adc-battery.h | 18 ------
> > 2 files changed, 4 insertions(+), 78 deletions(-)
> > delete mode 100644 include/linux/power/generic-adc-battery.h
> >
> > diff --git a/drivers/power/supply/generic-adc-battery.c b/drivers/power/supply/generic-adc-battery.c
> > index 771e5cfc49c3..d4f63d945b2c 100644
> > --- a/drivers/power/supply/generic-adc-battery.c
> > +++ b/drivers/power/supply/generic-adc-battery.c
> > @@ -22,7 +22,6 @@
> > #include <linux/slab.h>
> > #include <linux/iio/consumer.h>
> > #include <linux/iio/types.h>
> > -#include <linux/power/generic-adc-battery.h>
> > #include <linux/devm-helpers.h>
> > #define JITTER_DEFAULT 10 /* hope 10ms is enough */
> > @@ -48,9 +47,7 @@ struct gab {
> > struct power_supply *psy;
> > struct power_supply_desc psy_desc;
> > struct iio_channel *channel[GAB_MAX_CHAN_TYPE];
> > - struct gab_platform_data *pdata;
> > struct delayed_work bat_work;
> > - int level;
> > int status;
> > bool cable_plugged;
> > struct gpio_desc *charge_finished;
> > @@ -70,14 +67,6 @@ static void gab_ext_power_changed(struct power_supply *psy)
> > static const enum power_supply_property gab_props[] = {
> > POWER_SUPPLY_PROP_STATUS,
> > - POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
> > - POWER_SUPPLY_PROP_CHARGE_EMPTY_DESIGN,
> > - POWER_SUPPLY_PROP_VOLTAGE_NOW,
> > - POWER_SUPPLY_PROP_CURRENT_NOW,
> > - POWER_SUPPLY_PROP_TECHNOLOGY,
> > - POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
> > - POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
> > - POWER_SUPPLY_PROP_MODEL_NAME,
> > };
> > /*
> > @@ -97,17 +86,6 @@ static bool gab_charge_finished(struct gab *adc_bat)
> > return gpiod_get_value(adc_bat->charge_finished);
> > }
> > -static int gab_get_status(struct gab *adc_bat)
> > -{
> > - struct gab_platform_data *pdata = adc_bat->pdata;
> > - struct power_supply_info *bat_info;
> > -
> > - bat_info = &pdata->battery_info;
> > - if (adc_bat->level == bat_info->charge_full_design)
> > - return POWER_SUPPLY_STATUS_FULL;
>
> Not sure if this is intentional but I don't see the POWER_SUPPLY_STATUS_FULL
> being reported after applying your series. If this is intended, maybe it
> could be mentioned in commit log?
Right. I removed it, because 'level' is never written to and thus is
always 0. Apart from that the code needs to be a bit more flexible
anyways (i.e. should not just report full for exactly the same
value) and we currently do not have a way to calculate the current
charge value. Thus removing it makes sense at the moment. Something
better can be added once we have the required infrastructure.
-- Sebastian
Hi,
On Thu, Mar 16, 2023 at 09:10:03AM +0200, Matti Vaittinen wrote:
> > For chargers the situation is different (they usually want the
> > data before registration), but they should not expose the
> > battery data anyways.
>
> I probably should go back studying how the power-supply class
> works before continuing this discussion :)
>
> So, is it so that when a single IC contains both the charger logic
> and battery monitoring - then the driver is expected to create two
> power_supply devices. One for the battery and the other for the
> charger? I assume both of these pover_supply devices will then
> find the same battery_info - which means that one of the devices
> (probably the charger) should not auto-expose properties(?)
Yes.
> Well, as I said I should go study things better before continuing
> - but as I have limited time for studying this now I'll just ask
> if there is a danger we auto-expose battery details from existing
> drivers via two devices? And as I did no study I will just accept
> what ever answer you give and trust you to know this better ^_^;
Nothing will explode. But charger devices are supposed to provide
charger information and the data from simple-battery is about the
battery, so it should be exposed through a battery typed device.
Exposing e.g. POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN for a charger
makes no sense. Why would the charger have a design capacity?
> [...]
> > [05/12] power: supply: generic-adc-battery: drop jitter delay support
>
> I didn't feel technically capable of reviewing the above delay
> patch as I don't know what kind of delays are typically needed -
> or if they need to be configurable - or what is the purpose of
> this delay (some stabilization period after charging?)
>
> So, it's not that your patch had something I didn't agree with - I
> was just not feeling I understand the consequences of the changes.
> Purely from coding perspective it looked good to me :)
From what I can tell the original author had to debounce the GPIO.
-- Sebastian