Add DT support for generic ADC battery

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

Signed-off-by Sebastian Reichel <[email protected]>

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

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

Documentation/devicetree/bindings/power/supply/adc-battery.yaml new file +11 -0

new file mode 100644
index 000000000000..ed9702caedff
@@ -0,0 +1,70 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/supply/adc-battery.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ADC battery
+
+maintainers:
+  - Sebastian Reichel <[email protected]>
+

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

Signed-off-by Sebastian Reichel <[email protected]>

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

Signed-off-by Sebastian Reichel <[email protected]>

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

Signed-off-by Sebastian Reichel <[email protected]>

---
  drivers/power/supply/power_supply_core.c  | 173 +++++++++++++++++++---
  drivers/power/supply/power_supply_sysfs.c |  15 ++
  include/linux/power_supply.h              |   8 +
  3 files changed, 178 insertions(+), 18 deletions(-)

drivers/power/supply/power_supply_core.c +150 -8

index f3d7c1da299f..842c27de4fac 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,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;
-

drivers/power/supply/power_supply_core.c +0 -0

  	spin_lock_init(&psy->changed_lock);
  	rc = device_add(dev);
  	if (rc)

drivers/power/supply/power_supply_sysfs.c +5 -0

index c228205e0953..5842dfe5dfb7 100644
@@ -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;
 

drivers/power/supply/power_supply_sysfs.c +6 -0

+		ret = add_prop_uevent(dev, env, battery_props[j],
+			      prop_buf);
+		if (ret)
+			goto out;
+	}
+
  out:
  	free_page((unsigned long)prop_buf);
 

include/linux/power_supply.h +8 -0

index aa2c4a7c4826..a427f13c757f 100644
@@ -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 *

On 3/15/23 00:55, Sebastian Reichel wrote:
[...]

drivers/power/supply/power_supply_core.c __power_supply_register(struct device *parent, · line 1389

Quoted 7 lines

#ifdef CONFIG_THERMAL
static int power_supply_read_temp(struct thermal_zone_device *tzd,
int *temp)

drivers/power/supply/power_supply_core.c +1 -0

@@ -1255,6 +1387,11 @@ __power_supply_register(struct device *parent,
  		goto check_supplies_failed;
  	}
+	/* psy->battery_info is optional */

drivers/power/supply/power_supply_core.c __power_supply_register(struct device *parent, · line 1394

Quoted 4 lines

drivers/power/supply/power_supply_core.c +4 -0

+	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 :)

Quoted 25 lines

drivers/power/supply/power_supply_core.c +0 -0

  	spin_lock_init(&psy->changed_lock);
  	rc = device_add(dev);
  	if (rc)

drivers/power/supply/power_supply_sysfs.c +5 -0

index c228205e0953..5842dfe5dfb7 100644
@@ -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;

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

Quoted 5 lines

drivers/power/supply/power_supply_sysfs.c +4 -0

  	}
+	for (j = 0; j < power_supply_battery_info_properties_size; j++) {
+		if (!power_supply_battery_info_has_prop(psy->battery_info,
+				battery_props[j]))
+			continue;

Hmm. I just noticed that there can probably be same properties in the psy->desc->properties and in the battery-info.

I didn't cascade deep into the code so I can't say if it is a problem to add duplicates?

So, if this is safe, and if what I wrote above is not something you want to consider:

Reviewed-by Matti Vaittinen <[email protected]>

[...]

Hi,

Quoted 9 lines · Matti Vaittinen

On 3/15/23 00:55, Sebastian Reichel wrote:
[...]

drivers/power/supply/power_supply_core.c __power_supply_register(struct device *parent, · line 1389

Quoted 7 lines

#ifdef CONFIG_THERMAL
static int power_supply_read_temp(struct thermal_zone_device *tzd,
int *temp)

drivers/power/supply/power_supply_core.c +1 -0

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

Quoted 9 lines

drivers/power/supply/power_supply_core.c __power_supply_register(struct device *parent, · line 1394

Quoted 4 lines

drivers/power/supply/power_supply_core.c +4 -0

+	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 :)

Quoted 5 lines

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.

Quoted 7 lines

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.

Quoted 2 lines

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 :)

Quoted 6 lines

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

Quoted 25 lines

Quoted 25 lines

drivers/power/supply/power_supply_core.c +0 -0

   	spin_lock_init(&psy->changed_lock);
   	rc = device_add(dev);
   	if (rc)

drivers/power/supply/power_supply_sysfs.c +5 -0

index c228205e0953..5842dfe5dfb7 100644
@@ -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;

Quoted 8 lines

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

Quoted 5 lines

drivers/power/supply/power_supply_sysfs.c +4 -0

   	}
+	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.

Quoted 2 lines

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.

Quoted 3 lines

So, if this is safe, and if what I wrote above is not something you want to consider:

Quoted 1 line

Reviewed-by Matti Vaittinen <[email protected]>

Due to the changes I will not take this over in v3. Just to make sure - is it correct, that you do not want your R-b tag for the following two patches?

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

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

Quoted 2 lines

[...]

Thanks for your reviews,

Quoted 2 lines · Sebastian Reichel

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.

Quoted 5 lines

Quoted 3 lines

[...]

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

Quoted 3 lines

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 ^_^;

[...]

Quoted 1 line

[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 :)

Use standard simple-battery API for constant battery information like min and max voltage. This simplifies the driver a lot and brings automatic support for DT.

Reviewed-by Linus Walleij <[email protected]>

Signed-off-by Sebastian Reichel <[email protected]>

---
  drivers/power/supply/generic-adc-battery.c | 64 ++--------------------
  include/linux/power/generic-adc-battery.h  | 18 ------
  2 files changed, 4 insertions(+), 78 deletions(-)

delete mode 100644 include/linux/power/generic-adc-battery.h

drivers/power/supply/generic-adc-battery.c +0 -11

index 771e5cfc49c3..d4f63d945b2c 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);

Quoted 53 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.

Reviewed-by Linus Walleij <[email protected]>

Signed-off-by Sebastian Reichel <[email protected]>

---
  drivers/power/supply/generic-adc-battery.c | 64 ++--------------------
  include/linux/power/generic-adc-battery.h  | 18 ------
  2 files changed, 4 insertions(+), 78 deletions(-)

delete mode 100644 include/linux/power/generic-adc-battery.h

drivers/power/supply/generic-adc-battery.c +0 -15

index 771e5cfc49c3..d4f63d945b2c 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;

drivers/power/supply/generic-adc-battery.c static bool gab_charge_finished(struct gab *adc_bat) · line 107

Quoted 4 lines

drivers/power/supply/generic-adc-battery.c +0 -4

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

From: Sebastian Reichel <[email protected]>

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

Signed-off-by Sebastian Reichel <[email protected]>

---
  drivers/power/supply/generic-adc-battery.c | 7 ++++++-
  1 file changed, 6 insertions(+), 1 deletion(-)

drivers/power/supply/generic-adc-battery.c +6 -1

index 4811e72df8cd..0124d8d51af7 100644
@@ -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;
  	}

From: Sebastian Reichel <[email protected]>

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

Signed-off-by Sebastian Reichel <[email protected]>

Quoted 5 lines · Sebastian Reichel

From: Sebastian Reichel <[email protected]> Another typical thing to monitor via an ADC line is the battery temperature.

Signed-off-by Sebastian Reichel <[email protected]>

Reviewed-by Linus Walleij <[email protected]>

drivers/power/supply/generic-adc-battery.c static int gab_get_property(struct power_supply *psy, · line 122

Quoted 7 lines

static bool gab_charge_finished(struct gab *adc_bat)

drivers/power/supply/generic-adc-battery.c +2 -0

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