Skip to contentSkip to conversation

Add DT support for generic ADC battery

PATCH Superseded Sebastian Reichel 7 participants 40 messages Mar 9, 2023 – Mar 14, 2023

Subsystems (5)

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Sebastian Reichel

author maintainer
#0 lore raw

cover Add DT support for generic ADC battery

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.

-- Sebastian

Sebastian Reichel (11):
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 DT support
power: supply: generic-adc-battery: update copyright info

 .../bindings/power/supply/adc-battery.yaml    |  67 ++++++
 drivers/power/supply/generic-adc-battery.c    | 221 +++++-------------
 drivers/power/supply/power_supply_core.c      | 153 ++++++++++--
 drivers/power/supply/power_supply_sysfs.c     |  16 ++
 include/linux/power/generic-adc-battery.h     |  23 --
 include/linux/power_supply.h                  |  31 +++
 6 files changed, 301 insertions(+), 210 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/supply/adc-battery.yaml
 delete mode 100644 include/linux/power/generic-adc-battery.h
Signature · 1 line
2.39.2
author maintainer
#1 lore raw

PATCH 1/11 dt-bindings: power: supply: adc-battery: add binding

Add binding for a battery that is only monitored via ADC channels and simple status GPIOs.

---
 .../bindings/power/supply/adc-battery.yaml    | 67 +++++++++++++++++++
 1 file changed, 67 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/adc-battery.yaml
Documentation/devicetree/bindings/power/supply/adc-battery.yaml new file +67 -0
new file mode 100644
index 000000000000..9d478bf9d2ee
@@ -0,0 +1,67 @@
+# 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, which only reports (in circuit) voltage and optionally
+  current via an ADC channel.
+
+allOf:
+  - $ref: power-supply.yaml#
+
+properties:
+  compatible:
+    const: adc-battery
+
+  charged-gpios:
+    description:
+      GPIO which signals that the battery is fully charged.
+    maxItems: 1
+
+  io-channels:
+    minItems: 1
+    maxItems: 3
+
+  io-channel-names:
+    oneOf:
+      - const: voltage
+      - items:
+          - const: voltage
+          - enum:
+              - current
+              - power
+      - items:
+          - const: voltage
+          - const: current
+          - const: power
+
+  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>;
+    };
Signature · 1 line
2.39.2
#2 lore raw

Re: [PATCHv1 01/11] dt-bindings: power: supply: adc-battery: add binding

Hi Sebastian,

thanks for your patches!

Quoted 4 lines · Sebastian Reichel

Add binding for a battery that is only monitored via ADC channels and simple status GPIOs.

This does look very useful.

Documentation/devicetree/bindings/power/supply/adc-battery.yaml line 14

Quoted 8 lines
Documentation/devicetree/bindings/power/supply/adc-battery.yaml new file +8 -0
+title: ADC battery
+
+maintainers:
+  - Sebastian Reichel <[email protected]>
+
+description: |
+  Basic Battery, which only reports (in circuit) voltage and optionally
+  current via an ADC channel.

I would over-specify: "voltage over the terminals" and "current out of the battery" so this cannot be misunderstood.

+ this text:

It can also optionally indicate that the battery is full by pulling a GPIO line.

Documentation/devicetree/bindings/power/supply/adc-battery.yaml line 25

Quoted 3 lines
Documentation/devicetree/bindings/power/supply/adc-battery.yaml new file +3 -0
+  charged-gpios:
+    description:
+      GPIO which signals that the battery is fully charged.

It doesn't say how, I guess either this is an analog circuit (!) or a charger IC? If it doesn't matter, no big deal, but if something is implicit here, then spell it out please.

Quoted 1 line

+ fuel-gauge {

This techno-lingo/slang term is a bit unfortunate, but if there are precedents then stick with it.

The correct term could be something like battery-capacity-meter I suppose.

Yours,
Linus Walleij

author maintainer
#3 lore raw

Re: [PATCHv1 01/11] dt-bindings: power: supply: adc-battery: add binding

Hi Linus,

Quoted 4 lines · Linus Walleij

Hi Sebastian,

thanks for your patches!

Quoted 8 lines
Quoted 4 lines · Sebastian Reichel

Add binding for a battery that is only monitored via ADC channels and simple status GPIOs.

This does look very useful.

:)

Quoted 12 lines

Documentation/devicetree/bindings/power/supply/adc-battery.yaml line 14

Quoted 8 lines
Documentation/devicetree/bindings/power/supply/adc-battery.yaml new file +8 -0
+title: ADC battery
+
+maintainers:
+  - Sebastian Reichel <[email protected]>
+
+description: |
+  Basic Battery, which only reports (in circuit) voltage and optionally
+  current via an ADC channel.

I would over-specify: "voltage over the terminals" and "current out of the battery" so this cannot be misunderstood.

Quoted 4 lines

+ this text:

It can also optionally indicate that the battery is full by pulling a GPIO line.

Ack.

Quoted 8 lines

Documentation/devicetree/bindings/power/supply/adc-battery.yaml line 25

Quoted 4 lines
Documentation/devicetree/bindings/power/supply/adc-battery.yaml new file +3 -0
+  charged-gpios:
+    description:
+      GPIO which signals that the battery is fully charged.

It doesn't say how, I guess either this is an analog circuit (!) or a charger IC? If it doesn't matter, no big deal, but if something is implicit here, then spell it out please.

In my case the GPIO is provided by a charger chip, that is not software controllable (just reports charge-done & charger-connected via GPIOs). I've seen something similar in a customer device some years ago. I will add a sentence:

The GPIO is often provided by charger ICs, that are not software controllable.

Quoted 7 lines
Quoted 1 line

+ fuel-gauge {

This techno-lingo/slang term is a bit unfortunate, but if there are precedents then stick with it.

The correct term could be something like battery-capacity-meter I suppose.

Right now in DT we have

- specific node name (e.g. chip names) that should be changed :)
- smart-battery
- battery
- fuel-gauge

I think fuel-gauge is the most sensible of that list, considering hardware vendors usually call their chips battery fuel gauge.

-- Sebastian

#4 lore raw

Re: [PATCH v2] power: supply: da9150: Fix use after free bug in da9150_charger_remove due to race condition

Quoted 34 lines · Sebastian Reichel <[email protected]> 于2023年3月13日周一 06:31写道:

Hi,

Quoted 22 lines · Zheng Wang

In da9150_charger_probe, &charger->otg_work is bound with da9150_charger_otg_work. da9150_charger_otg_ncb may be called to start the work.

If we remove the module which will call da9150_charger_remove to make cleanup, there may be a unfinished work. The possible sequence is as follows:

Fix it by canceling the work before cleanup in the da9150_charger_remove

CPU0 CPUc1

                    |da9150_charger_otg_work

da9150_charger_remove |
power_supply_unregister |
device_unregister |
power_supply_dev_release|

kfree(psy)          |
                    |
                    |         power_supply_changed(charger->usb);
                    |   //use
Quoted 7 lines
Fixes c1a281e34dae ("power: Add support for DA9150 Charger")

---
v2:
- fix wrong description in commit message and mov cancel_work_sync after usb_unregister_notifier suggested by Sebastian Reichel
---

Quoted 3 lines

Thanks, queued to power-supply's fixes branch. Please make sure you send your patches to the correct destination next time (linux-scsi should be linux-pm).

Thanks for your effort. I'll keep that in mind :)

Best regards,
Zheng

Quoted 19 lines
Quoted 19 lines
 drivers/power/supply/da9150-charger.c | 1 +
 1 file changed, 1 insertion(+)
drivers/power/supply/da9150-charger.c +1 -1
index 14da5c595dd9..a87aeaea38e1 100644
@@ -657,6 +657,7 @@ static int da9150_charger_remove(struct platform_device *pdev)
 
      if (!IS_ERR_OR_NULL(charger->usb_phy))
              usb_unregister_notifier(charger->usb_phy, &charger->otg_nb);
+     cancel_work_sync(&charger->otg_work);
 
      power_supply_unregister(charger->battery);
      power_supply_unregister(charger->usb);
--

2.25.1

#5 lore raw

Re: [PATCHv1 04/11] power: supply: generic-adc-battery: fix unit scaling

In reply to Sebastian Reichel
Quoted 3 lines · Sebastian Reichel

There is no mainline board using this driver and I think there never was one. I did add a Fixes tag now, but its probably not worth any backporting trouble considering it has no users.

Good point. If a tree falls in the wood an no-one is there to hear it, it doesn't make a sound.

Yours,
Linus Walleij

#6 lore raw

Re: [PATCHv1 01/11] dt-bindings: power: supply: adc-battery: add binding

In reply to Sebastian Reichel
Quoted 4 lines · Sebastian Reichel

Add binding for a battery that is only monitored via ADC channels and simple status GPIOs.

Thank you for your patch. There is something to discuss/improve.

Documentation/devicetree/bindings/power/supply/adc-battery.yaml line 12

Quoted 5 lines
Documentation/devicetree/bindings/power/supply/adc-battery.yaml new file +5 -0
+
+maintainers:
+  - Sebastian Reichel <[email protected]>
+
+description: |

Don't need '|'.

Quoted 10 lines
Documentation/devicetree/bindings/power/supply/adc-battery.yaml new file +10 -0
+  Basic Battery, which only reports (in circuit) voltage and optionally
+  current via an ADC channel.
+
+allOf:
+  - $ref: power-supply.yaml#
+
+properties:
+  compatible:
+    const: adc-battery
+

Documentation/devicetree/bindings/power/supply/adc-battery.yaml line 32

Quoted 10 lines
Documentation/devicetree/bindings/power/supply/adc-battery.yaml new file +10 -0
+  charged-gpios:
+    description:
+      GPIO which signals that the battery is fully charged.
+    maxItems: 1
+
+  io-channels:
+    minItems: 1
+    maxItems: 3
+
+  io-channel-names:

Simpler:

minItems: 1
items:
- const: voltage
- enum: [ current, power ]
- const: power

Documentation/devicetree/bindings/power/supply/adc-battery.yaml line 44

Quoted 12 lines
Documentation/devicetree/bindings/power/supply/adc-battery.yaml new file +12 -0
+    oneOf:
+      - const: voltage
+      - items:
+          - const: voltage
+          - enum:
+              - current
+              - power
+      - items:
+          - const: voltage
+          - const: current
+          - const: power
+

What about temperature? For max17040 this was recently proposed and I wonder whether it is desirable.

https://lore.kernel.org/all/[email protected]/

Best regards,
Krzysztof

#7 lore raw

Re: [PATCHv1 01/11] dt-bindings: power: supply: adc-battery: add binding

In reply to Sebastian Reichel

Hi Sebastian,

Quoted 48 lines · Sebastian Reichel

Add binding for a battery that is only monitored via ADC channels and simple status GPIOs.

---
  .../bindings/power/supply/adc-battery.yaml    | 67 +++++++++++++++++++
  1 file changed, 67 insertions(+)

create mode 100644 Documentation/devicetree/bindings/power/supply/adc-battery.yaml

Documentation/devicetree/bindings/power/supply/adc-battery.yaml new file +33 -0
new file mode 100644
index 000000000000..9d478bf9d2ee
@@ -0,0 +1,67 @@
+# 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, which only reports (in circuit) voltage and optionally
+  current via an ADC channel.
+
+allOf:
+  - $ref: power-supply.yaml#
+
+properties:
+  compatible:
+    const: adc-battery
+
+  charged-gpios:
+    description:
+      GPIO which signals that the battery is fully charged.
+    maxItems: 1
+
+  io-channels:
+    minItems: 1
+    maxItems: 3
+
+  io-channel-names:
+    oneOf:

Documentation/devicetree/bindings/power/supply/adc-battery.yaml line 43

Quoted 10 lines
Documentation/devicetree/bindings/power/supply/adc-battery.yaml new file +10 -0
+      - const: voltage
+      - items:
+          - const: voltage
+          - enum:
+              - current
+              - power
+      - items:
+          - const: voltage
+          - const: current
+          - const: power

Good side of not knowing things is being able to asking for more information ;)

So, just by judging these bindings, we have a battery which provides fuel-gauge information via analog line connected to ADC(?)

Reading the description you have here and comments by Linus allows me to assume the line can represent current flowing out of the battery, or the battery voltage.

My guess then is that the io-channel-names property is going to tell which if these properties is being informed by the specific lines, right(?). Do you think you could add some small description for io-channel-names if you respin the series? I'd like to be more certain I "guessed" things right. ;) Maybe also add the 'power' option in the main description which currently just states voltage and power. (Assuming some devices do actually "expose" power levels via these "channels"?

Quoted 24 lines
Documentation/devicetree/bindings/power/supply/adc-battery.yaml new file +24 -0
+
+  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>;
+    };
Signature · 5 lines
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~
author maintainer
#8 lore raw

PATCH 2/11 power: supply: core: auto-exposure of simple-battery data

In reply to Sebastian Reichel

Add support for automatically exposing data from the simple-battery firmware node with a single configuration option in the power-supply device.

---
 drivers/power/supply/power_supply_core.c  | 153 +++++++++++++++++++---
 drivers/power/supply/power_supply_sysfs.c |  16 +++
 include/linux/power_supply.h              |  31 +++++
 3 files changed, 181 insertions(+), 19 deletions(-)
drivers/power/supply/power_supply_core.c +134 -19
index f3d7c1da299f..c3684ec46b3f 100644
@@ -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,111 @@ void power_supply_put_battery_info(struct power_supply *psy,
 }
 EXPORT_SYMBOL_GPL(power_supply_put_battery_info);
 
+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 +1151,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,9 +1177,13 @@ 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(psy->desc->expose_battery_info)
+		return power_supply_battery_info_get_prop(psy->battery_info, psp, val);
+	else
+		return -EINVAL;
 }
-EXPORT_SYMBOL_GPL(power_supply_get_property);
 
 int power_supply_set_property(struct power_supply *psy,
 			    enum power_supply_property psp,
@@ -1117,22 +1242,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 +1364,12 @@ __power_supply_register(struct device *parent,
 		goto check_supplies_failed;
 	}
 
+	if (psy->desc->expose_battery_info) {
+		rc = power_supply_get_battery_info(psy, &psy->battery_info);
+		if (rc)
+			goto check_supplies_failed;
+	}
+
 	spin_lock_init(&psy->changed_lock);
 	rc = device_add(dev);
 	if (rc)
drivers/power/supply/power_supply_sysfs.c +16 -0
index c228205e0953..8822a17f9589 100644
@@ -380,6 +380,11 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
 		}
 	}
 
+	if (psy->desc->expose_battery_info) {
+		if (power_supply_battery_info_has_prop(psy->battery_info, attrno))
+			return mode;
+	}
+
 	return 0;
 }
 
@@ -488,6 +493,17 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
 			goto out;
 	}
 
+	if (psy->desc->expose_battery_info) {
+		for (j = 0; j < ARRAY_SIZE(power_supply_battery_info_properties); j++) {
+			if (!power_supply_battery_info_has_prop(psy->battery_info, power_supply_battery_info_properties[j]))
+				continue;
+			ret = add_prop_uevent(dev, env, power_supply_battery_info_properties[j],
+				      prop_buf);
+			if (ret)
+				goto out;
+		}
+	}
+
 out:
 	free_page((unsigned long)prop_buf);
 
include/linux/power_supply.h +31 -0
index aa2c4a7c4826..de0ea8320f3d 100644
@@ -275,6 +275,13 @@ struct power_supply_desc {
 	 * sensors or other supplies.
 	 */
 	bool no_thermal;
+	/*
+	 * Set if constant battery information from firmware should be
+	 * exposed automatically. No driver specific code is required
+	 * in that case. If the driver also handles a property provided
+	 * by constant firmware data, the driver's handler is preferred.
+	 */
+	bool expose_battery_info;
 	/* For APM emulation, think legacy userspace. */
 	int use_for_apm;
 };
@@ -301,6 +308,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;
@@ -766,6 +774,24 @@ struct power_supply_battery_info {
 	int bti_resistance_tolerance;
 };
 
+static 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,
+};
+
 extern struct atomic_notifier_head power_supply_notifier;
 extern int power_supply_reg_notifier(struct notifier_block *nb);
 extern void power_supply_unreg_notifier(struct notifier_block *nb);
@@ -795,6 +821,11 @@ 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 *
Signature · 1 line
2.39.2
#9 lore raw

Re: [PATCHv1 02/11] power: supply: core: auto-exposure of simple-battery data

I love your patch! Perhaps something to improve:

Hi Sebastian,

I love your patch! Perhaps something to improve:

[auto build test WARNING on sre-power-supply/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sebastian-Reichel/dt-bindings-power-supply-adc-battery-add-binding/20230310-065229
base:   https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git for-next
patch link:    https://lore.kernel.org/r/20230309225041.477440-3-sre%40kernel.org
patch subject: [PATCHv1 02/11] power: supply: core: auto-exposure of simple-battery data
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230310/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0

reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/83ec4c841d68a66bc95f5e534a805e765785f934
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Sebastian-Reichel/dt-bindings-power-supply-adc-battery-add-binding/20230310-065229 git checkout 83ec4c841d68a66bc95f5e534a805e765785f934
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 olddefconfig
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/power/supply/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

In file included from drivers/power/supply/power_supply_core.c:21:

>> include/linux/power_supply.h:777:41: warning: 'power_supply_battery_info_properties' defined but not used [-Wunused-const-variable=]
     777 | static const enum power_supply_property power_supply_battery_info_properties[] = {
         |                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +/power_supply_battery_info_properties +777 include/linux/power_supply.h

776

 > 777	static const enum power_supply_property power_supply_battery_info_properties[] = {

778 POWER_SUPPLY_PROP_TECHNOLOGY,
779 POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
780 POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
781 POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
782 POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
783 POWER_SUPPLY_PROP_PRECHARGE_CURRENT,
784 POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT,
785 POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
786 POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
787 POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN,
788 POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX,
789 POWER_SUPPLY_PROP_TEMP_ALERT_MIN,
790 POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
791 POWER_SUPPLY_PROP_TEMP_MIN,
792 POWER_SUPPLY_PROP_TEMP_MAX,
793 };
794

Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
#10 lore raw

Re: [PATCHv1 02/11] power: supply: core: auto-exposure of simple-battery data

In reply to Sebastian Reichel
I love your patch! Yet something to improve:

Hi Sebastian,

I love your patch! Yet something to improve:

[auto build test ERROR on sre-power-supply/for-next]
[also build test ERROR on robh/for-next linus/master v6.3-rc1 next-20230310] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sebastian-Reichel/dt-bindings-power-supply-adc-battery-add-binding/20230310-065229
base:   https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git for-next
patch link:    https://lore.kernel.org/r/20230309225041.477440-3-sre%40kernel.org
patch subject: [PATCHv1 02/11] power: supply: core: auto-exposure of simple-battery data
config: x86_64-randconfig-a002 (https://download.01.org/0day-ci/archive/20230310/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/83ec4c841d68a66bc95f5e534a805e765785f934
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Sebastian-Reichel/dt-bindings-power-supply-adc-battery-add-binding/20230310-065229
        git checkout 83ec4c841d68a66bc95f5e534a805e765785f934
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 olddefconfig
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "power_supply_get_property" [drivers/phy/ti/phy-tusb1210.ko] undefined!
>> ERROR: modpost: "power_supply_get_property" [drivers/power/supply/88pm860x_charger.ko] undefined!
>> ERROR: modpost: "power_supply_get_property" [drivers/power/supply/charger-manager.ko] undefined!
Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Reviewed-by on v1 2/11
#11 lore raw

Re: [PATCHv1 02/11] power: supply: core: auto-exposure of simple-battery data

In reply to Sebastian Reichel

Hi Sebastian,

thanks for your patch!

include/linux/power_supply.h struct power_supply_desc { · line 284

Quoted 7 lines · Sebastian Reichel
include/linux/power_supply.h +7 -0
+       /*
+        * Set if constant battery information from firmware should be
+        * exposed automatically. No driver specific code is required
+        * in that case. If the driver also handles a property provided
+        * by constant firmware data, the driver's handler is preferred.
+        */
+       bool expose_battery_info;

Playing it safe with opt-in I see! But I would probably invert it and add a hide_battery_info for those that don't wanna expose it. It seems pretty useful to just expose this in general.

However I have no insight in what happens on laptops etc for this so I guess you have your reasons, either way:

include/linux/power_supply.h extern int power_supply_get_battery_info(struct power_supply *psy, · line 828

Quoted 5 lines
include/linux/power_supply.h +5 -0
+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);

I think the build robots complain because you need to add some stubs for the not enabled case.

Yours,
Linus Walleij

Reviewed-by on v1 2/11
#12 lore raw

Re: [PATCHv1 02/11] power: supply: core: auto-exposure of simple-battery data

In reply to Sebastian Reichel
Quoted 122 lines · Sebastian Reichel

Add support for automatically exposing data from the simple-battery firmware node with a single configuration option in the power-supply device.

---
  drivers/power/supply/power_supply_core.c  | 153 +++++++++++++++++++---
  drivers/power/supply/power_supply_sysfs.c |  16 +++
  include/linux/power_supply.h              |  31 +++++
  3 files changed, 181 insertions(+), 19 deletions(-)
drivers/power/supply/power_supply_core.c +95 -1
index f3d7c1da299f..c3684ec46b3f 100644
@@ -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,111 @@ void power_supply_put_battery_info(struct power_supply *psy,
  }
  EXPORT_SYMBOL_GPL(power_supply_put_battery_info);
 
+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:

drivers/power/supply/power_supply_core.c void power_supply_put_battery_info(struct power_supply *psy, · line 938

Quoted 10 lines
drivers/power/supply/power_supply_core.c +10 -0
+			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);

This is not really relevant for the series. Just speaking it as it came into my mind - I am not expecting changes to this series.

I do very much like the way you have these battery-info APIs not requiring the "struct power_supply *psy". It may be useful for drivers to get the stuff from battery-node prior registering to the power-supply core. I think it'd be nice to have a 'power-supply-info getter API like power_supply_get_battery_info() - which does not require the struct power_supply * but just a device pointer or fwnode pointer. I think it might also be reasonable to pull the battery-info parsing APIs in own file. Or maybe not - definitely up-to you guys. I don't have any active psy-stuff at my hands right now :)

Quoted 96 lines
drivers/power/supply/power_supply_core.c +29 -18
+
  /**
   * power_supply_temp2resist_simple() - find the battery internal resistance
   * percent from temperature
@@ -1046,6 +1151,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,9 +1177,13 @@ 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(psy->desc->expose_battery_info)
+		return power_supply_battery_info_get_prop(psy->battery_info, psp, val);
+	else
+		return -EINVAL;
  }
-EXPORT_SYMBOL_GPL(power_supply_get_property);
 
  int power_supply_set_property(struct power_supply *psy,
  			    enum power_supply_property psp,
@@ -1117,22 +1242,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 +1364,12 @@ __power_supply_register(struct device *parent,
  		goto check_supplies_failed;
  	}
 
+	if (psy->desc->expose_battery_info) {
+		rc = power_supply_get_battery_info(psy, &psy->battery_info);
+		if (rc)
+			goto check_supplies_failed;
+	}
+
  	spin_lock_init(&psy->changed_lock);
  	rc = device_add(dev);
  	if (rc)
drivers/power/supply/power_supply_sysfs.c +5 -0
index c228205e0953..8822a17f9589 100644
@@ -380,6 +380,11 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
  		}
  	}
 
+	if (psy->desc->expose_battery_info) {
+		if (power_supply_battery_info_has_prop(psy->battery_info, attrno))
+			return mode;
+	}
+
  	return 0;
  }
 

drivers/power/supply/power_supply_sysfs.c int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env) · line 501

Quoted 9 lines
drivers/power/supply/power_supply_sysfs.c +6 -0
@@ -488,6 +493,17 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
  			goto out;
  	}
 
+	if (psy->desc->expose_battery_info) {
+		for (j = 0; j < ARRAY_SIZE(power_supply_battery_info_properties); j++) {
+			if (!power_supply_battery_info_has_prop(psy->battery_info, power_supply_battery_info_properties[j]))
+				continue;
+			ret = add_prop_uevent(dev, env, power_supply_battery_info_properties[j],
+				      prop_buf);

Usually I do not spot styling things like this - but for some reason it now caught my attention. If you for some reason respin, then you might want to either do this an oneliner - or split the longer line "power_supply_battery_info_has_prop(..." just above.

With or without that:

Signature · 5 lines
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~
author maintainer
#13 lore raw

PATCH 3/11 power: supply: generic-adc-battery: convert to managed resources

In reply to Sebastian Reichel

Convert driver to use managed resources to simplify driver code.

---
 drivers/power/supply/generic-adc-battery.c | 81 ++++++----------------
 1 file changed, 23 insertions(+), 58 deletions(-)
drivers/power/supply/generic-adc-battery.c +23 -58
index 66039c665dd1..917bd2a6cc52 100644
@@ -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);
 
Signature · 1 line
2.39.2
Reviewed-by on v1 3/11
#15 lore raw

Re: [PATCHv1 03/11] power: supply: generic-adc-battery: convert to managed resources

In reply to Sebastian Reichel

Hi Sebastian, All

To me this does look like a great simplification :)

Quoted 3 lines · Sebastian Reichel

Convert driver to use managed resources to simplify driver code.

Signature · 5 lines
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~
author maintainer
#16 lore raw

PATCH 4/11 power: supply: generic-adc-battery: fix unit scaling

In reply to Sebastian Reichel

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.

---
 drivers/power/supply/generic-adc-battery.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/power/supply/generic-adc-battery.c +3 -0
index 917bd2a6cc52..535972a332b3 100644
@@ -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;
 }
 
Signature · 1 line
2.39.2
Reviewed-by on v1 4/11
#17 lore raw

Re: [PATCHv1 04/11] power: supply: generic-adc-battery: fix unit scaling

Quoted 5 lines · Sebastian Reichel

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 tag?

This code can not have seen much testing.

Yours,
Linus Walleij

Reviewed-by on v1 4/11
#18 lore raw

Re: [PATCHv1 04/11] power: supply: generic-adc-battery: fix unit scaling

In reply to Sebastian Reichel
Quoted 5 lines · Sebastian Reichel

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.

As Linus wrote, Fixes-tag would be good. With that remark:

Quoted 18 lines
---
  drivers/power/supply/generic-adc-battery.c | 3 +++
  1 file changed, 3 insertions(+)
drivers/power/supply/generic-adc-battery.c +3 -0
index 917bd2a6cc52..535972a332b3 100644
@@ -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;
  }
 
Signature · 5 lines
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~
author maintainer
#19 lore raw

PATCH 5/11 power: supply: generic-adc-battery: drop jitter delay support

In reply to Sebastian Reichel

Drop support for configuring IRQ jitter delay by using big enough fixed value.

---
 drivers/power/supply/generic-adc-battery.c | 13 ++++---------
 include/linux/power/generic-adc-battery.h  |  3 ---
 2 files changed, 4 insertions(+), 12 deletions(-)
drivers/power/supply/generic-adc-battery.c +4 -9
index 535972a332b3..e20894460d7f 100644
@@ -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;
 }
 
include/linux/power/generic-adc-battery.h +0 -3
index c68cbf34cd34..50eb4bf28286 100644
@@ -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 */
Signature · 1 line
2.39.2
author maintainer
#21 lore raw

PATCH 6/11 power: supply: generic-adc-battery: drop charge now support

In reply to Sebastian Reichel

Drop CHARGE_NOW support, which requires a platform specific calculation method.

---
 drivers/power/supply/generic-adc-battery.c | 4 ----
 include/linux/power/generic-adc-battery.h  | 2 --
 2 files changed, 6 deletions(-)
drivers/power/supply/generic-adc-battery.c +0 -4
index e20894460d7f..d07eeb7d46d3 100644
@@ -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:
include/linux/power/generic-adc-battery.h +0 -2
index 50eb4bf28286..54434e4304d3 100644
@@ -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 */
Signature · 1 line
2.39.2
Reviewed-by on v1 6/11
#22 lore raw

Re: [PATCHv1 06/11] power: supply: generic-adc-battery: drop charge now support

Quoted 4 lines · Sebastian Reichel

Drop CHARGE_NOW support, which requires a platform specific calculation method.

I agree. If we want to support this, we should use the generic methods with interpolation tables defined in DT as well, and it also ideally requires load compensated resistance calculation to figure out Ri so this can bring any kind of reasonable precision.

Yours,
Linus Walleij

Reviewed-by
#23 lore raw

Re: [PATCHv1 06/11] power: supply: generic-adc-battery: drop charge now support

Hi dee Ho all,

Quoted 11 lines · Linus Walleij
Quoted 4 lines · Sebastian Reichel

Drop CHARGE_NOW support, which requires a platform specific calculation method.

I agree. If we want to support this, we should use the generic methods with interpolation tables defined in DT as well, and it also ideally requires load compensated resistance calculation to figure out Ri so this can bring any kind of reasonable precision.

I guess you have your reasons, besides you have far better insight to things than I do - hence I am not really objecting this - just asking a question ;)

Do we have generic facilities of computing this based on the DT tables / Ri in place(?) I guess that we do need/see platform specific implementations as long as there is no generic "de-facto" way of doing this available...

Well, maybe this helps kicking things to that direction :)

-- Matti

Signature · 5 lines
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~
#24 lore raw

Re: [PATCHv1 06/11] power: supply: generic-adc-battery: drop charge now support

Quoted 13 lines · Matti Vaittinen
Quoted 11 lines · Linus Walleij
Quoted 4 lines · Sebastian Reichel

Drop CHARGE_NOW support, which requires a platform specific calculation method.

I agree. If we want to support this, we should use the generic methods with interpolation tables defined in DT as well, and it also ideally requires load compensated resistance calculation to figure out Ri so this can bring any kind of reasonable precision.

Quoted 6 lines

I guess you have your reasons, besides you have far better insight to things than I do - hence I am not really objecting this - just asking a question ;)

Do we have generic facilities of computing this based on the DT tables / Ri in place(?)

Not yet, for the Samsung batteries I used a static look-up table derived from the compatible string for calculating Ri from VBAT and from that calculate the capacity from estimated open circuit voltage, see drivers/power/supply/samsung-sdi-battery.c

Quoted 3 lines

I guess that we do need/see platform specific implementations as long as there is no generic "de-facto" way of doing this available...

The method I used with Samsung batteries is fine as long as all you need to know to know everything about a battery is the compatible string. Pretty much any Lion battery with a clearly defined product name can be done this way.

The only reason to put the interpolation tables into the device tree would be to support any random battery, such as one that you do not know the model or this can change.

I am however mildly sceptic about adding that: if you know the VBAT-to-Ri and OCV-to-capacity tables, you must have a datasheet, and then you know the name of the battery product and hence you know the right compatible string...

I think the right way to handle any capacity curves for any battery would be to create static data like I did for the Samsung batteries.

Yours,
Linus Walleij

author maintainer
#25 lore raw

PATCH 7/11 power: supply: generic-adc-battery: drop memory alloc error message

In reply to Sebastian Reichel

Error printing happens automatically for memory allocation problems.

---
 drivers/power/supply/generic-adc-battery.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/power/supply/generic-adc-battery.c +1 -3
index d07eeb7d46d3..771e5cfc49c3 100644
@@ -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;
Signature · 1 line
2.39.2
Reviewed-by on v1 7/11
#27 lore raw

Re: [PATCHv1 07/11] power: supply: generic-adc-battery: drop memory alloc error message

In reply to Sebastian Reichel
Quoted 3 lines · Sebastian Reichel

Error printing happens automatically for memory allocation problems.

Quoted 20 lines
---
  drivers/power/supply/generic-adc-battery.c | 4 +---
  1 file changed, 1 insertion(+), 3 deletions(-)
drivers/power/supply/generic-adc-battery.c +1 -3
index d07eeb7d46d3..771e5cfc49c3 100644
@@ -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;
Signature · 5 lines
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~
author maintainer
#28 lore raw

PATCH 8/11 power: supply: generic-adc-battery: use simple-battery API

In reply to Sebastian Reichel

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.

---
 drivers/power/supply/generic-adc-battery.c | 65 ++--------------------
 include/linux/power/generic-adc-battery.h  | 18 ------
 2 files changed, 5 insertions(+), 78 deletions(-)
 delete mode 100644 include/linux/power/generic-adc-battery.h
drivers/power/supply/generic-adc-battery.c +5 -60
index 771e5cfc49c3..fc6fcfda1ef2 100644
@@ -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
@@ -309,6 +253,7 @@ static int gab_probe(struct platform_device *pdev)
 	 */
 	psy_desc->properties = properties;
 	psy_desc->num_properties = index;
+	psy_desc->expose_battery_info = true;
 
 	adc_bat->psy = devm_power_supply_register(&pdev->dev, psy_desc, &psy_cfg);
 	if (IS_ERR(adc_bat->psy))
include/linux/power/generic-adc-battery.h deleted +0 -18
deleted file mode 100644
index 54434e4304d3..000000000000
@@ -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 */
Signature · 1 line
2.39.2
Reviewed-by on v1 8/11
#29 lore raw

Re: [PATCHv1 08/11] power: supply: generic-adc-battery: use simple-battery API

Quoted 5 lines · Sebastian Reichel

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.

That's neat.

Yours,
Linus Walleij

author maintainer
#30 lore raw

PATCH 9/11 power: supply: generic-adc-battery: simplify read_channel logic

In reply to Sebastian Reichel

Drop mostly useless gab_prop_to_chan() function by directly supplying the correct enum value to read_channel().

---
 drivers/power/supply/generic-adc-battery.c | 31 ++++------------------
 1 file changed, 5 insertions(+), 26 deletions(-)
drivers/power/supply/generic-adc-battery.c +5 -26
index fc6fcfda1ef2..7bc54566664f 100644
@@ -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;
 	}
Signature · 1 line
2.39.2
Reviewed-by on v1 9/11
#32 lore raw

Re: [PATCHv1 09/11] power: supply: generic-adc-battery: simplify read_channel logic

In reply to Sebastian Reichel

Hi,

Looks good to me.

Quoted 4 lines · Sebastian Reichel

Drop mostly useless gab_prop_to_chan() function by directly supplying the correct enum value to read_channel().

Signature · 5 lines
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~
author maintainer
#33 lore raw

PATCH 10/11 power: supply: generic-adc-battery: add DT support

In reply to Sebastian Reichel

This adds full DT support to the driver. Because of the previous changes just adding a compatible value is enough.

---
 drivers/power/supply/generic-adc-battery.c | 9 +++++++++
 1 file changed, 9 insertions(+)
drivers/power/supply/generic-adc-battery.c +9 -0
index 7bc54566664f..436e75d226ed 100644
@@ -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 */
@@ -170,6 +171,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);
@@ -284,10 +286,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,
 };
Signature · 1 line
2.39.2
Reviewed-by on v1 10/11
#35 lore raw

Re: [PATCHv1 10/11] power: supply: generic-adc-battery: add DT support

In reply to Sebastian Reichel
Quoted 4 lines · Sebastian Reichel

This adds full DT support to the driver. Because of the previous changes just adding a compatible value is enough.

Best Regards,
-- Matti

Signature · 5 lines
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~
author maintainer
#36 lore raw

PATCH 11/11 power: supply: generic-adc-battery: update copyright info

In reply to Sebastian Reichel

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.

---
 drivers/power/supply/generic-adc-battery.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)
drivers/power/supply/generic-adc-battery.c +3 -8
index 436e75d226ed..ac72140dc136 100644
@@ -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>
Signature · 1 line
2.39.2
Reviewed-by on v1 11/11
#37 lore raw

Re: [PATCHv1 11/11] power: supply: generic-adc-battery: update copyright info

Quoted 8 lines · Sebastian Reichel

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.

Yours,
Linus Walleij

Reviewed-by on v1 11/11
#38 lore raw

Re: [PATCHv1 11/11] power: supply: generic-adc-battery: update copyright info

In reply to Sebastian Reichel
Quoted 8 lines · Sebastian Reichel

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.

Quoted 25 lines
---
  drivers/power/supply/generic-adc-battery.c | 11 +++--------
  1 file changed, 3 insertions(+), 8 deletions(-)
drivers/power/supply/generic-adc-battery.c +3 -8
index 436e75d226ed..ac72140dc136 100644
@@ -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>
Signature · 5 lines
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~
#39 lore raw

PATCH v2 1/1 power: supply: da9150: Fix use after free bug in da9150_charger_remove due to race condition

In da9150_charger_probe, &charger->otg_work is bound with da9150_charger_otg_work. da9150_charger_otg_ncb may be called to start the work.

If we remove the module which will call da9150_charger_remove to make cleanup, there may be a unfinished work. The possible sequence is as follows:

Fix it by canceling the work before cleanup in the da9150_charger_remove

CPU0 CPUc1

                    |da9150_charger_otg_work

da9150_charger_remove |
power_supply_unregister |
device_unregister |
power_supply_dev_release|

kfree(psy)          |
                    |
                    | 	power_supply_changed(charger->usb);
                    |   //use
Fixes c1a281e34dae ("power: Add support for DA9150 Charger")

---
v2:
- fix wrong description in commit message and mov cancel_work_sync after usb_unregister_notifier suggested by Sebastian Reichel

---
 drivers/power/supply/da9150-charger.c | 1 +
 1 file changed, 1 insertion(+)
drivers/power/supply/da9150-charger.c +1 -0
index 14da5c595dd9..a87aeaea38e1 100644
@@ -657,6 +657,7 @@ static int da9150_charger_remove(struct platform_device *pdev)
 
 	if (!IS_ERR_OR_NULL(charger->usb_phy))
 		usb_unregister_notifier(charger->usb_phy, &charger->otg_nb);
+	cancel_work_sync(&charger->otg_work);
 
 	power_supply_unregister(charger->battery);
 	power_supply_unregister(charger->usb);
Signature · 1 line
2.25.1