[PATCHv2 00/12] Add DT support for generic ADC battery

[PATCHv2 00/12] Add DT support for generic ADC battery

[Sebastian Reichel]

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

[PATCHv2 01/12] dt-bindings: power: supply: adc-battery: add binding

[Sebastian Reichel]

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

Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 .../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 <sre@kernel.org>
+
+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

[PATCHv2 02/12] power: supply: core: auto-exposure of simple-battery data

[Sebastian Reichel]

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 <sre@kernel.org>
---
 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

[PATCHv2 03/12] power: supply: generic-adc-battery: convert to managed resources

[Sebastian Reichel]

Convert driver to use managed resources to simplify driver code.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 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

[PATCHv2 04/12] power: supply: generic-adc-battery: fix unit scaling

[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: e60fea794e6e ("power: battery: Generic battery driver using IIO")
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 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

[PATCHv2 05/12] power: supply: generic-adc-battery: drop jitter delay support

[Sebastian Reichel]

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

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 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

[PATCHv2 06/12] power: supply: generic-adc-battery: drop charge now support

[Sebastian Reichel]

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

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 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

[PATCHv2 07/12] power: supply: generic-adc-battery: drop memory alloc error message

[Sebastian Reichel]

Error printing happens automatically for memory allocation problems.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 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

[PATCHv2 08/12] power: supply: generic-adc-battery: use simple-battery API

[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.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 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 <anish198519851985@gmail.com>
- */
-
-#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

[PATCHv2 09/12] power: supply: generic-adc-battery: simplify read_channel logic

[Sebastian Reichel]

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

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 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

[PATCHv2 10/12] power: supply: generic-adc-battery: add temperature support

[Sebastian Reichel]

From: Sebastian Reichel <sebastian.reichel@collabora.com>

Another typical thing to monitor via an ADC line is
the battery temperature.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 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

[PATCHv2 11/12] power: supply: generic-adc-battery: add DT support

[Sebastian Reichel]

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

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 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

[PATCHv2 12/12] power: supply: generic-adc-battery: update copyright info

[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.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 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 <anish198519851985@gmail.com>
- * 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 <sre@kernel.org>
  */
 #include <linux/interrupt.h>
 #include <linux/platform_device.h>
-- 
2.39.2

Re: [PATCHv2 01/12] dt-bindings: power: supply: adc-battery: add binding

[Matti Vaittinen]

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 <sre@kernel.org>
> ---
>   .../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 <sre@kernel.org>
> +
> +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 <mazziesaccount@gmail.com>

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCHv2 01/12] dt-bindings: power: supply: adc-battery: add binding

[Krzysztof Kozlowski]

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 <sre@kernel.org>


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

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

[Matti Vaittinen]

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 <sre@kernel.org>
> ---
>   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 <mazziesaccount@gmail.com>

> +		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! ~~

Re: [PATCHv2 10/12] power: supply: generic-adc-battery: add temperature support

[Matti Vaittinen]

On 3/15/23 00:55, Sebastian Reichel wrote:
> From: Sebastian Reichel <sebastian.reichel@collabora.com>
> 
> Another typical thing to monitor via an ADC line is
> the battery temperature.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>

> ---
>   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! ~~

Re: [PATCHv2 01/12] dt-bindings: power: supply: adc-battery: add binding

[Linus Walleij]

On Tue, Mar 14, 2023 at 11:55 PM Sebastian Reichel <sre@kernel.org> wrote:

> Add binding for a battery that is only monitored via ADC
> channels and simple status GPIOs.
>
> Signed-off-by: Sebastian Reichel <sre@kernel.org>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCHv2 10/12] power: supply: generic-adc-battery: add temperature support

[Linus Walleij]

On Tue, Mar 14, 2023 at 11:55 PM Sebastian Reichel <sre@kernel.org> wrote:

> From: Sebastian Reichel <sebastian.reichel@collabora.com>
>
> Another typical thing to monitor via an ADC line is
> the battery temperature.
>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

>  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

Re: [PATCHv2 10/12] power: supply: generic-adc-battery: add temperature support

[Sebastian Reichel]

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

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 <sre@kernel.org> wrote:
> > From: Sebastian Reichel <sebastian.reichel@collabora.com>
> > Another typical thing to monitor via an ADC line is
> > the battery temperature.
> >
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> >  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

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

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

[Sebastian Reichel]

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

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 <mazziesaccount@gmail.com>

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

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

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

[Matti Vaittinen]

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 <mazziesaccount@gmail.com>
> 
> 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! ~~

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

[Matti Vaittinen]

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! ~~

Re: [PATCHv2 08/12] power: supply: generic-adc-battery: use simple-battery API

[Matti Vaittinen]

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 <linus.walleij@linaro.org>
> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> ---
>   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! ~~

Re: [PATCHv2 08/12] power: supply: generic-adc-battery: use simple-battery API

[Sebastian Reichel]

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

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 <linus.walleij@linaro.org>
> > Signed-off-by: Sebastian Reichel <sre@kernel.org>
> > ---
> >   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

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

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

[Sebastian Reichel]

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

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

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