[PATCHv1 00/11] Add DT support for generic ADC battery

[PATCHv1 00/11] 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.

-- Sebastian

Sebastian Reichel (11):
  dt-bindings: power: supply: adc-battery: add binding
  power: supply: core: auto-exposure of simple-battery data
  power: supply: generic-adc-battery: convert to managed resources
  power: supply: generic-adc-battery: fix unit scaling
  power: supply: generic-adc-battery: drop jitter delay support
  power: supply: generic-adc-battery: drop charge now support
  power: supply: generic-adc-battery: drop memory alloc error message
  power: supply: generic-adc-battery: use simple-battery API
  power: supply: generic-adc-battery: simplify read_channel logic
  power: supply: generic-adc-battery: add DT support
  power: supply: generic-adc-battery: update copyright info

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

-- 
2.39.2

[PATCHv1 01/11] 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    | 67 +++++++++++++++++++
 1 file changed, 67 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..9d478bf9d2ee
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/adc-battery.yaml
@@ -0,0 +1,67 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/supply/adc-battery.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ADC battery
+
+maintainers:
+  - Sebastian Reichel <sre@kernel.org>
+
+description: |
+  Basic Battery, which only reports (in circuit) voltage and optionally
+  current via an ADC channel.
+
+allOf:
+  - $ref: power-supply.yaml#
+
+properties:
+  compatible:
+    const: adc-battery
+
+  charged-gpios:
+    description:
+      GPIO which signals that the battery is fully charged.
+    maxItems: 1
+
+  io-channels:
+    minItems: 1
+    maxItems: 3
+
+  io-channel-names:
+    oneOf:
+      - const: voltage
+      - items:
+          - const: voltage
+          - enum:
+              - current
+              - power
+      - items:
+          - const: voltage
+          - const: current
+          - const: power
+
+  monitored-battery: true
+
+required:
+  - compatible
+  - io-channels
+  - io-channel-names
+  - monitored-battery
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    fuel-gauge {
+        compatible = "adc-battery";
+        charged-gpios = <&gpio 42 GPIO_ACTIVE_HIGH>;
+        io-channels = <&adc 13>, <&adc 37>;
+        io-channel-names = "voltage", "current";
+
+        power-supplies = <&charger>;
+        monitored-battery = <&battery>;
+    };
-- 
2.39.2

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

[Linus Walleij]

Hi Sebastian,

thanks for your patches!

On Thu, Mar 9, 2023 at 11:50 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>

This does look very useful.

> +title: ADC battery
> +
> +maintainers:
> +  - Sebastian Reichel <sre@kernel.org>
> +
> +description: |
> +  Basic Battery, which only reports (in circuit) voltage and optionally
> +  current via an ADC channel.

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

+ this text:

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

> +  charged-gpios:
> +    description:
> +      GPIO which signals that the battery is fully charged.

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

> +    fuel-gauge {

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

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

Yours,
Linus Walleij

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

[Sebastian Reichel]

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

Hi Linus,

On Fri, Mar 10, 2023 at 09:14:39AM +0100, Linus Walleij wrote:
> Hi Sebastian,
> 
> thanks for your patches!
> 
> On Thu, Mar 9, 2023 at 11:50 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>
> 
> This does look very useful.

:)

> > +title: ADC battery
> > +
> > +maintainers:
> > +  - Sebastian Reichel <sre@kernel.org>
> > +
> > +description: |
> > +  Basic Battery, which only reports (in circuit) voltage and optionally
> > +  current via an ADC channel.
> 
> I would over-specify: "voltage over the terminals" and
> "current out of the battery" so this cannot be misunderstood.
> 
> + this text:
> 
> It can also optionally indicate that the battery is full by pulling a GPIO
> line.

Ack.

> 
> > +  charged-gpios:
> > +    description:
> > +      GPIO which signals that the battery is fully charged.
> 
> It doesn't say how, I guess either this is an analog circuit (!) or
> a charger IC? If it doesn't matter, no big deal, but if something is
> implicit here, then spell it out please.

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

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

> > +    fuel-gauge {
> 
> This techno-lingo/slang term is a bit unfortunate, but if there are
> precedents then stick with it.
> 
> The correct term could be something like battery-capacity-meter
> I suppose.

Right now in DT we have

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

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

-- Sebastian

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

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

[Sebastian Reichel]

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

Hi,

On Fri, Mar 10, 2023 at 09:20:09AM +0100, Linus Walleij wrote:
> On Thu, Mar 9, 2023 at 11:50 PM Sebastian Reichel <sre@kernel.org> wrote:
> 
> > +       /*
> > +        * Set if constant battery information from firmware should be
> > +        * exposed automatically. No driver specific code is required
> > +        * in that case. If the driver also handles a property provided
> > +        * by constant firmware data, the driver's handler is preferred.
> > +        */
> > +       bool expose_battery_info;
> 
> Playing it safe with opt-in I see! But I would probably invert it and
> add a hide_battery_info for those that don't wanna expose it. It seems
> pretty useful to just expose this in general.

I just did not yet spend the time to understand if there are any
issues. I guess I can do it now and then remove the opt-in part.

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

ACPI based systems should be fine, since battery info does not
yet support ACPI and thus nothing changes for them.

> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> > +extern bool power_supply_battery_info_has_prop(struct power_supply_battery_info *info,
> > +                                              enum power_supply_property psp);
> > +extern int power_supply_battery_info_get_prop(struct power_supply_battery_info *info,
> > +                                             enum power_supply_property psp,
> > +                                             union power_supply_propval *val);
> 
> I think the build robots complain because you need to add some stubs
> for the not enabled case.

I don't think so. They are only used from code needing POWER_SUPPLY
being enabled.

One reported error is about the array of battery_info properties not
being used when POWER_SUPPLY is disabled. I will move that array to a
better place.

The other error is about power_supply_get_property(), because I
accidently removed the EXPORT_SYMBOL_GPL(power_supply_get_property).
I did not notice myself, because I compiled a monolithic kernel for
the thermal camera for easy deployment.

Thanks for the review, much appreciated!

-- Sebastian

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

Re: [PATCH] power: supply: charger-manager: Use of_property_read_bool() for boolean properties

[Sebastian Reichel]

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

Hi,

On Fri, Mar 10, 2023 at 08:47:35AM -0600, Rob Herring wrote:
> It is preferred to use typed property access functions (i.e.
> of_property_read_<type> functions) rather than low-level
> of_get_property/of_find_property functions for reading properties.
> Convert reading boolean properties to to of_property_read_bool().
> 
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---

Thanks, queued.

-- Sebastian

>  drivers/power/supply/charger-manager.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/charger-manager.c b/drivers/power/supply/charger-manager.c
> index c9e8450c646f..5fa6ba7f41e1 100644
> --- a/drivers/power/supply/charger-manager.c
> +++ b/drivers/power/supply/charger-manager.c
> @@ -1331,7 +1331,7 @@ static struct charger_desc *of_cm_parse_desc(struct device *dev)
>  	of_property_read_string(np, "cm-thermal-zone", &desc->thermal_zone);
>  
>  	of_property_read_u32(np, "cm-battery-cold", &desc->temp_min);
> -	if (of_get_property(np, "cm-battery-cold-in-minus", NULL))
> +	if (of_property_read_bool(np, "cm-battery-cold-in-minus"))
>  		desc->temp_min *= -1;
>  	of_property_read_u32(np, "cm-battery-hot", &desc->temp_max);
>  	of_property_read_u32(np, "cm-battery-temp-diff", &desc->temp_diff);
> -- 
> 2.39.2
> 

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

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

[Sebastian Reichel]

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

Hi,

On Fri, Mar 10, 2023 at 09:23:06AM +0100, Linus Walleij wrote:
> On Thu, Mar 9, 2023 at 11:50 PM Sebastian Reichel <sre@kernel.org> wrote:
> 
> > 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.
> >
> > Signed-off-by: Sebastian Reichel <sre@kernel.org>
> 
> Fixes: tag?
> Cc: stable@vger.kernel.org
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> This code can not have seen much testing.

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

-- Sebastian

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

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

[Linus Walleij]

On Tue, Mar 14, 2023 at 12:17 AM Sebastian Reichel <sre@kernel.org> wrote:

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

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

Yours,
Linus Walleij

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

[Krzysztof Kozlowski]

On 09/03/2023 23:50, 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>

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


> +
> +maintainers:
> +  - Sebastian Reichel <sre@kernel.org>
> +
> +description: |

Don't need '|'.

> +  Basic Battery, which only reports (in circuit) voltage and optionally
> +  current via an ADC channel.
> +
> +allOf:
> +  - $ref: power-supply.yaml#
> +
> +properties:
> +  compatible:
> +    const: adc-battery
> +
> +  charged-gpios:
> +    description:
> +      GPIO which signals that the battery is fully charged.
> +    maxItems: 1
> +
> +  io-channels:
> +    minItems: 1
> +    maxItems: 3
> +
> +  io-channel-names:

Simpler:

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

> +    oneOf:
> +      - const: voltage
> +      - items:
> +          - const: voltage
> +          - enum:
> +              - current
> +              - power
> +      - items:
> +          - const: voltage
> +          - const: current
> +          - const: power
> +

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

https://lore.kernel.org/all/74ba115e-9838-4983-7b93-188a8260dd8a@linaro.org/

Best regards,
Krzysztof

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

[Matti Vaittinen]

Hi Sebastian,

On 3/10/23 00:50, 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    | 67 +++++++++++++++++++
>   1 file changed, 67 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..9d478bf9d2ee
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/adc-battery.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/supply/adc-battery.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ADC battery
> +
> +maintainers:
> +  - Sebastian Reichel <sre@kernel.org>
> +
> +description: |
> +  Basic Battery, which only reports (in circuit) voltage and optionally
> +  current via an ADC channel.
> +
> +allOf:
> +  - $ref: power-supply.yaml#
> +
> +properties:
> +  compatible:
> +    const: adc-battery
> +
> +  charged-gpios:
> +    description:
> +      GPIO which signals that the battery is fully charged.
> +    maxItems: 1
> +
> +  io-channels:
> +    minItems: 1
> +    maxItems: 3
> +
> +  io-channel-names:
> +    oneOf:
> +      - const: voltage
> +      - items:
> +          - const: voltage
> +          - enum:
> +              - current
> +              - power
> +      - items:
> +          - const: voltage
> +          - const: current
> +          - const: power

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

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

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

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

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

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

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

[PATCHv1 02/11] 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  | 153 +++++++++++++++++++---
 drivers/power/supply/power_supply_sysfs.c |  16 +++
 include/linux/power_supply.h              |  31 +++++
 3 files changed, 181 insertions(+), 19 deletions(-)

diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index f3d7c1da299f..c3684ec46b3f 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,111 @@ void power_supply_put_battery_info(struct power_supply *psy,
 }
 EXPORT_SYMBOL_GPL(power_supply_put_battery_info);
 
+bool power_supply_battery_info_has_prop(struct power_supply_battery_info *info,
+				        enum power_supply_property psp)
+{
+	if (!info)
+		return false;
+
+	switch (psp) {
+		case POWER_SUPPLY_PROP_TECHNOLOGY:
+			return info->technology != POWER_SUPPLY_TECHNOLOGY_UNKNOWN;
+		case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN:
+			return info->energy_full_design_uwh >= 0;
+		case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
+			return info->charge_full_design_uah >= 0;
+		case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
+			return info->voltage_min_design_uv >= 0;
+		case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
+			return info->voltage_max_design_uv >= 0;
+		case POWER_SUPPLY_PROP_PRECHARGE_CURRENT:
+			return info->precharge_current_ua >= 0;
+		case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT:
+			return info->charge_term_current_ua >= 0;
+		case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
+			return info->constant_charge_current_max_ua >= 0;
+		case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
+			return info->constant_charge_voltage_max_uv >= 0;
+		case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN:
+			return info->temp_ambient_alert_min > INT_MIN;
+		case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX:
+			return info->temp_ambient_alert_max < INT_MAX;
+		case POWER_SUPPLY_PROP_TEMP_ALERT_MIN:
+			return info->temp_alert_min > INT_MIN;
+		case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
+			return info->temp_alert_max < INT_MAX;
+		case POWER_SUPPLY_PROP_TEMP_MIN:
+			return info->temp_min > INT_MIN;
+		case POWER_SUPPLY_PROP_TEMP_MAX:
+			return info->temp_max < INT_MAX;
+		default:
+			return false;
+	}
+}
+EXPORT_SYMBOL_GPL(power_supply_battery_info_has_prop);
+
+int power_supply_battery_info_get_prop(struct power_supply_battery_info *info,
+				       enum power_supply_property psp,
+				       union power_supply_propval *val)
+{
+	if (!info)
+		return -EINVAL;
+
+	if (!power_supply_battery_info_has_prop(info, psp))
+		return -EINVAL;
+
+	switch (psp) {
+		case POWER_SUPPLY_PROP_TECHNOLOGY:
+			val->intval = info->technology;
+			return 0;
+		case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN:
+			val->intval = info->energy_full_design_uwh;
+			return 0;
+		case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
+			val->intval = info->charge_full_design_uah;
+			return 0;
+		case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
+			val->intval = info->voltage_min_design_uv;
+			return 0;
+		case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
+			val->intval = info->voltage_max_design_uv;
+			return 0;
+		case POWER_SUPPLY_PROP_PRECHARGE_CURRENT:
+			val->intval = info->precharge_current_ua;
+			return 0;
+		case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT:
+			val->intval = info->charge_term_current_ua;
+			return 0;
+		case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
+			val->intval = info->constant_charge_current_max_ua;
+			return 0;
+		case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
+			val->intval = info->constant_charge_voltage_max_uv;
+			return 0;
+		case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN:
+			val->intval = info->temp_ambient_alert_min;
+			return 0;
+		case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX:
+			val->intval = info->temp_ambient_alert_max;
+			return 0;
+		case POWER_SUPPLY_PROP_TEMP_ALERT_MIN:
+			val->intval = info->temp_alert_min;
+			return 0;
+		case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
+			val->intval = info->temp_alert_max;
+			return 0;
+		case POWER_SUPPLY_PROP_TEMP_MIN:
+			val->intval = info->temp_min;
+			return 0;
+		case POWER_SUPPLY_PROP_TEMP_MAX:
+			val->intval = info->temp_max;
+			return 0;
+		default:
+			return -EINVAL;
+	}
+}
+EXPORT_SYMBOL_GPL(power_supply_battery_info_get_prop);
+
 /**
  * power_supply_temp2resist_simple() - find the battery internal resistance
  * percent from temperature
@@ -1046,6 +1151,22 @@ bool power_supply_battery_bti_in_range(struct power_supply_battery_info *info,
 }
 EXPORT_SYMBOL_GPL(power_supply_battery_bti_in_range);
 
+static bool psy_has_property(const struct power_supply_desc *psy_desc,
+			     enum power_supply_property psp)
+{
+	bool found = false;
+	int i;
+
+	for (i = 0; i < psy_desc->num_properties; i++) {
+		if (psy_desc->properties[i] == psp) {
+			found = true;
+			break;
+		}
+	}
+
+	return found;
+}
+
 int power_supply_get_property(struct power_supply *psy,
 			    enum power_supply_property psp,
 			    union power_supply_propval *val)
@@ -1056,9 +1177,13 @@ int power_supply_get_property(struct power_supply *psy,
 		return -ENODEV;
 	}
 
-	return psy->desc->get_property(psy, psp, val);
+	if (psy_has_property(psy->desc, psp))
+		return psy->desc->get_property(psy, psp, val);
+	else if(psy->desc->expose_battery_info)
+		return power_supply_battery_info_get_prop(psy->battery_info, psp, val);
+	else
+		return -EINVAL;
 }
-EXPORT_SYMBOL_GPL(power_supply_get_property);
 
 int power_supply_set_property(struct power_supply *psy,
 			    enum power_supply_property psp,
@@ -1117,22 +1242,6 @@ void power_supply_unreg_notifier(struct notifier_block *nb)
 }
 EXPORT_SYMBOL_GPL(power_supply_unreg_notifier);
 
-static bool psy_has_property(const struct power_supply_desc *psy_desc,
-			     enum power_supply_property psp)
-{
-	bool found = false;
-	int i;
-
-	for (i = 0; i < psy_desc->num_properties; i++) {
-		if (psy_desc->properties[i] == psp) {
-			found = true;
-			break;
-		}
-	}
-
-	return found;
-}
-
 #ifdef CONFIG_THERMAL
 static int power_supply_read_temp(struct thermal_zone_device *tzd,
 		int *temp)
@@ -1255,6 +1364,12 @@ __power_supply_register(struct device *parent,
 		goto check_supplies_failed;
 	}
 
+	if (psy->desc->expose_battery_info) {
+		rc = power_supply_get_battery_info(psy, &psy->battery_info);
+		if (rc)
+			goto check_supplies_failed;
+	}
+
 	spin_lock_init(&psy->changed_lock);
 	rc = device_add(dev);
 	if (rc)
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index c228205e0953..8822a17f9589 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -380,6 +380,11 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
 		}
 	}
 
+	if (psy->desc->expose_battery_info) {
+		if (power_supply_battery_info_has_prop(psy->battery_info, attrno))
+			return mode;
+	}
+
 	return 0;
 }
 
@@ -488,6 +493,17 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
 			goto out;
 	}
 
+	if (psy->desc->expose_battery_info) {
+		for (j = 0; j < ARRAY_SIZE(power_supply_battery_info_properties); j++) {
+			if (!power_supply_battery_info_has_prop(psy->battery_info, power_supply_battery_info_properties[j]))
+				continue;
+			ret = add_prop_uevent(dev, env, power_supply_battery_info_properties[j],
+				      prop_buf);
+			if (ret)
+				goto out;
+		}
+	}
+
 out:
 	free_page((unsigned long)prop_buf);
 
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index aa2c4a7c4826..de0ea8320f3d 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -275,6 +275,13 @@ struct power_supply_desc {
 	 * sensors or other supplies.
 	 */
 	bool no_thermal;
+	/*
+	 * Set if constant battery information from firmware should be
+	 * exposed automatically. No driver specific code is required
+	 * in that case. If the driver also handles a property provided
+	 * by constant firmware data, the driver's handler is preferred.
+	 */
+	bool expose_battery_info;
 	/* For APM emulation, think legacy userspace. */
 	int use_for_apm;
 };
@@ -301,6 +308,7 @@ struct power_supply {
 	bool initialized;
 	bool removing;
 	atomic_t use_cnt;
+	struct power_supply_battery_info *battery_info;
 #ifdef CONFIG_THERMAL
 	struct thermal_zone_device *tzd;
 	struct thermal_cooling_device *tcd;
@@ -766,6 +774,24 @@ struct power_supply_battery_info {
 	int bti_resistance_tolerance;
 };
 
+static const enum power_supply_property power_supply_battery_info_properties[] = {
+	POWER_SUPPLY_PROP_TECHNOLOGY,
+	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
+	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
+	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
+	POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
+	POWER_SUPPLY_PROP_PRECHARGE_CURRENT,
+	POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT,
+	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
+	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
+	POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN,
+	POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX,
+	POWER_SUPPLY_PROP_TEMP_ALERT_MIN,
+	POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
+	POWER_SUPPLY_PROP_TEMP_MIN,
+	POWER_SUPPLY_PROP_TEMP_MAX,
+};
+
 extern struct atomic_notifier_head power_supply_notifier;
 extern int power_supply_reg_notifier(struct notifier_block *nb);
 extern void power_supply_unreg_notifier(struct notifier_block *nb);
@@ -795,6 +821,11 @@ extern int power_supply_get_battery_info(struct power_supply *psy,
 					 struct power_supply_battery_info **info_out);
 extern void power_supply_put_battery_info(struct power_supply *psy,
 					  struct power_supply_battery_info *info);
+extern bool power_supply_battery_info_has_prop(struct power_supply_battery_info *info,
+					       enum power_supply_property psp);
+extern int power_supply_battery_info_get_prop(struct power_supply_battery_info *info,
+					      enum power_supply_property psp,
+					      union power_supply_propval *val);
 extern int power_supply_ocv2cap_simple(struct power_supply_battery_ocv_table *table,
 				       int table_len, int ocv);
 extern struct power_supply_battery_ocv_table *
-- 
2.39.2

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

[kernel test robot]

Hi Sebastian,

I love your patch! Perhaps something to improve:

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

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

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303100854.V2YFWYZu-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/power/supply/power_supply_core.c:21:
>> include/linux/power_supply.h:777:41: warning: 'power_supply_battery_info_properties' defined but not used [-Wunused-const-variable=]
     777 | static const enum power_supply_property power_supply_battery_info_properties[] = {
         |                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


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

   776	
 > 777	static const enum power_supply_property power_supply_battery_info_properties[] = {
   778		POWER_SUPPLY_PROP_TECHNOLOGY,
   779		POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
   780		POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
   781		POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
   782		POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
   783		POWER_SUPPLY_PROP_PRECHARGE_CURRENT,
   784		POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT,
   785		POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
   786		POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
   787		POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN,
   788		POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX,
   789		POWER_SUPPLY_PROP_TEMP_ALERT_MIN,
   790		POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
   791		POWER_SUPPLY_PROP_TEMP_MIN,
   792		POWER_SUPPLY_PROP_TEMP_MAX,
   793	};
   794	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

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

[kernel test robot]

Hi Sebastian,

I love your patch! Yet something to improve:

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

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

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303101236.fqHwh3Tt-lkp@intel.com/

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

>> ERROR: modpost: "power_supply_get_property" [drivers/phy/ti/phy-tusb1210.ko] undefined!
>> ERROR: modpost: "power_supply_get_property" [drivers/power/supply/88pm860x_charger.ko] undefined!
>> ERROR: modpost: "power_supply_get_property" [drivers/power/supply/charger-manager.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

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

[Linus Walleij]

Hi Sebastian,

thanks for your patch!

On Thu, Mar 9, 2023 at 11:50 PM Sebastian Reichel <sre@kernel.org> wrote:

> +       /*
> +        * Set if constant battery information from firmware should be
> +        * exposed automatically. No driver specific code is required
> +        * in that case. If the driver also handles a property provided
> +        * by constant firmware data, the driver's handler is preferred.
> +        */
> +       bool expose_battery_info;

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

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

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

> +extern bool power_supply_battery_info_has_prop(struct power_supply_battery_info *info,
> +                                              enum power_supply_property psp);
> +extern int power_supply_battery_info_get_prop(struct power_supply_battery_info *info,
> +                                             enum power_supply_property psp,
> +                                             union power_supply_propval *val);

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

Yours,
Linus Walleij

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

[Matti Vaittinen]

On 3/10/23 00:50, 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  | 153 +++++++++++++++++++---
>   drivers/power/supply/power_supply_sysfs.c |  16 +++
>   include/linux/power_supply.h              |  31 +++++
>   3 files changed, 181 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index f3d7c1da299f..c3684ec46b3f 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,111 @@ void power_supply_put_battery_info(struct power_supply *psy,
>   }
>   EXPORT_SYMBOL_GPL(power_supply_put_battery_info);
>   
> +bool power_supply_battery_info_has_prop(struct power_supply_battery_info *info,
> +				        enum power_supply_property psp)
> +{
> +	if (!info)
> +		return false;
> +
> +	switch (psp) {
> +		case POWER_SUPPLY_PROP_TECHNOLOGY:
> +			return info->technology != POWER_SUPPLY_TECHNOLOGY_UNKNOWN;
> +		case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN:
> +			return info->energy_full_design_uwh >= 0;
> +		case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> +			return info->charge_full_design_uah >= 0;
> +		case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
> +			return info->voltage_min_design_uv >= 0;
> +		case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
> +			return info->voltage_max_design_uv >= 0;
> +		case POWER_SUPPLY_PROP_PRECHARGE_CURRENT:
> +			return info->precharge_current_ua >= 0;
> +		case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT:
> +			return info->charge_term_current_ua >= 0;
> +		case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
> +			return info->constant_charge_current_max_ua >= 0;
> +		case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
> +			return info->constant_charge_voltage_max_uv >= 0;
> +		case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN:
> +			return info->temp_ambient_alert_min > INT_MIN;
> +		case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX:
> +			return info->temp_ambient_alert_max < INT_MAX;
> +		case POWER_SUPPLY_PROP_TEMP_ALERT_MIN:
> +			return info->temp_alert_min > INT_MIN;
> +		case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
> +			return info->temp_alert_max < INT_MAX;
> +		case POWER_SUPPLY_PROP_TEMP_MIN:
> +			return info->temp_min > INT_MIN;
> +		case POWER_SUPPLY_PROP_TEMP_MAX:
> +			return info->temp_max < INT_MAX;
> +		default:
> +			return false;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(power_supply_battery_info_has_prop);
> +
> +int power_supply_battery_info_get_prop(struct power_supply_battery_info *info,
> +				       enum power_supply_property psp,
> +				       union power_supply_propval *val)
> +{
> +	if (!info)
> +		return -EINVAL;
> +
> +	if (!power_supply_battery_info_has_prop(info, psp))
> +		return -EINVAL;
> +
> +	switch (psp) {
> +		case POWER_SUPPLY_PROP_TECHNOLOGY:
> +			val->intval = info->technology;
> +			return 0;
> +		case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN:
> +			val->intval = info->energy_full_design_uwh;
> +			return 0;
> +		case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> +			val->intval = info->charge_full_design_uah;
> +			return 0;
> +		case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
> +			val->intval = info->voltage_min_design_uv;
> +			return 0;
> +		case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
> +			val->intval = info->voltage_max_design_uv;
> +			return 0;
> +		case POWER_SUPPLY_PROP_PRECHARGE_CURRENT:
> +			val->intval = info->precharge_current_ua;
> +			return 0;
> +		case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT:
> +			val->intval = info->charge_term_current_ua;
> +			return 0;
> +		case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
> +			val->intval = info->constant_charge_current_max_ua;
> +			return 0;
> +		case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
> +			val->intval = info->constant_charge_voltage_max_uv;
> +			return 0;
> +		case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN:
> +			val->intval = info->temp_ambient_alert_min;
> +			return 0;
> +		case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX:
> +			val->intval = info->temp_ambient_alert_max;
> +			return 0;
> +		case POWER_SUPPLY_PROP_TEMP_ALERT_MIN:
> +			val->intval = info->temp_alert_min;
> +			return 0;
> +		case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
> +			val->intval = info->temp_alert_max;
> +			return 0;
> +		case POWER_SUPPLY_PROP_TEMP_MIN:
> +			val->intval = info->temp_min;
> +			return 0;
> +		case POWER_SUPPLY_PROP_TEMP_MAX:
> +			val->intval = info->temp_max;
> +			return 0;
> +		default:
> +			return -EINVAL;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(power_supply_battery_info_get_prop);

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

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

> +
>   /**
>    * power_supply_temp2resist_simple() - find the battery internal resistance
>    * percent from temperature
> @@ -1046,6 +1151,22 @@ bool power_supply_battery_bti_in_range(struct power_supply_battery_info *info,
>   }
>   EXPORT_SYMBOL_GPL(power_supply_battery_bti_in_range);
>   
> +static bool psy_has_property(const struct power_supply_desc *psy_desc,
> +			     enum power_supply_property psp)
> +{
> +	bool found = false;
> +	int i;
> +
> +	for (i = 0; i < psy_desc->num_properties; i++) {
> +		if (psy_desc->properties[i] == psp) {
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	return found;
> +}
> +
>   int power_supply_get_property(struct power_supply *psy,
>   			    enum power_supply_property psp,
>   			    union power_supply_propval *val)
> @@ -1056,9 +1177,13 @@ int power_supply_get_property(struct power_supply *psy,
>   		return -ENODEV;
>   	}
>   
> -	return psy->desc->get_property(psy, psp, val);
> +	if (psy_has_property(psy->desc, psp))
> +		return psy->desc->get_property(psy, psp, val);
> +	else if(psy->desc->expose_battery_info)
> +		return power_supply_battery_info_get_prop(psy->battery_info, psp, val);
> +	else
> +		return -EINVAL;
>   }
> -EXPORT_SYMBOL_GPL(power_supply_get_property);
>   
>   int power_supply_set_property(struct power_supply *psy,
>   			    enum power_supply_property psp,
> @@ -1117,22 +1242,6 @@ void power_supply_unreg_notifier(struct notifier_block *nb)
>   }
>   EXPORT_SYMBOL_GPL(power_supply_unreg_notifier);
>   
> -static bool psy_has_property(const struct power_supply_desc *psy_desc,
> -			     enum power_supply_property psp)
> -{
> -	bool found = false;
> -	int i;
> -
> -	for (i = 0; i < psy_desc->num_properties; i++) {
> -		if (psy_desc->properties[i] == psp) {
> -			found = true;
> -			break;
> -		}
> -	}
> -
> -	return found;
> -}
> -
>   #ifdef CONFIG_THERMAL
>   static int power_supply_read_temp(struct thermal_zone_device *tzd,
>   		int *temp)
> @@ -1255,6 +1364,12 @@ __power_supply_register(struct device *parent,
>   		goto check_supplies_failed;
>   	}
>   
> +	if (psy->desc->expose_battery_info) {
> +		rc = power_supply_get_battery_info(psy, &psy->battery_info);
> +		if (rc)
> +			goto check_supplies_failed;
> +	}
> +
>   	spin_lock_init(&psy->changed_lock);
>   	rc = device_add(dev);
>   	if (rc)
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index c228205e0953..8822a17f9589 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -380,6 +380,11 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
>   		}
>   	}
>   
> +	if (psy->desc->expose_battery_info) {
> +		if (power_supply_battery_info_has_prop(psy->battery_info, attrno))
> +			return mode;
> +	}
> +
>   	return 0;
>   }
>   
> @@ -488,6 +493,17 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
>   			goto out;
>   	}
>   
> +	if (psy->desc->expose_battery_info) {
> +		for (j = 0; j < ARRAY_SIZE(power_supply_battery_info_properties); j++) {
> +			if (!power_supply_battery_info_has_prop(psy->battery_info, power_supply_battery_info_properties[j]))
> +				continue;
> +			ret = add_prop_uevent(dev, env, power_supply_battery_info_properties[j],
> +				      prop_buf);

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

With or without that:
Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>



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

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

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

[Sebastian Reichel]

Convert driver to use managed resources to simplify driver code.

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

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

[Linus Walleij]

On Thu, Mar 9, 2023 at 11:50 PM Sebastian Reichel <sre@kernel.org> wrote:

> Convert driver to use managed resources to simplify driver code.
>
> Signed-off-by: Sebastian Reichel <sre@kernel.org>

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

Yours,
Linus Walleij

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

[Matti Vaittinen]

Hi Sebastian, All

To me this does look like a great simplification :)

On 3/10/23 00:50, Sebastian Reichel wrote:
> Convert driver to use managed resources to simplify driver code.
> 
> Signed-off-by: Sebastian Reichel <sre@kernel.org>

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



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

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

[PATCHv1 04/11] 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.

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

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

[Linus Walleij]

On Thu, Mar 9, 2023 at 11:50 PM Sebastian Reichel <sre@kernel.org> wrote:

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

Fixes: tag?
Cc: stable@vger.kernel.org

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

This code can not have seen much testing.

Yours,
Linus Walleij

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

[Matti Vaittinen]

On 3/10/23 00:50, Sebastian Reichel wrote:
> 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.
> 
> Signed-off-by: Sebastian Reichel <sre@kernel.org>

As Linus wrote, Fixes-tag would be good. With that remark:
Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>

> ---
>   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;
>   }
>   

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

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

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

[Sebastian Reichel]

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

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

Re: [PATCHv1 05/11] power: supply: generic-adc-battery: drop jitter delay support

[Linus Walleij]

On Thu, Mar 9, 2023 at 11:50 PM Sebastian Reichel <sre@kernel.org> wrote:

> Drop support for configuring IRQ jitter delay by using big
> enough fixed value.
>
> Signed-off-by: Sebastian Reichel <sre@kernel.org>

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

Yours,
Linus Walleij

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

[Sebastian Reichel]

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

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

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

[Linus Walleij]

On Thu, Mar 9, 2023 at 11:50 PM Sebastian Reichel <sre@kernel.org> wrote:

> Drop CHARGE_NOW support, which requires a platform specific
> calculation method.
>
> Signed-off-by: Sebastian Reichel <sre@kernel.org>

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

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

Yours,
Linus Walleij

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

[Matti Vaittinen]

Hi dee Ho all,

On 3/10/23 10:29, Linus Walleij wrote:
> On Thu, Mar 9, 2023 at 11:50 PM Sebastian Reichel <sre@kernel.org> wrote:
> 
>> Drop CHARGE_NOW support, which requires a platform specific
>> calculation method.
>>
>> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> 
> I agree. If we want to support this, we should use the generic
> methods with interpolation tables defined in DT as well, and it also
> ideally requires load compensated resistance calculation to figure
> out Ri so this can bring any kind of reasonable precision.

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

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

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

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


-- Matti


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

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

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

[Linus Walleij]

On Mon, Mar 13, 2023 at 8:49 AM Matti Vaittinen
<mazziesaccount@gmail.com> wrote:
> On 3/10/23 10:29, Linus Walleij wrote:
> > On Thu, Mar 9, 2023 at 11:50 PM Sebastian Reichel <sre@kernel.org> wrote:
> >
> >> Drop CHARGE_NOW support, which requires a platform specific
> >> calculation method.
> >>
> >> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> >
> > I agree. If we want to support this, we should use the generic
> > methods with interpolation tables defined in DT as well, and it also
> > ideally requires load compensated resistance calculation to figure
> > out Ri so this can bring any kind of reasonable precision.
>
> I guess you have your reasons, besides you have far better insight to
> things than I do - hence I am not really objecting this - just asking a
> question ;)
>
> Do we have generic facilities of computing this based on the DT tables /
> Ri in place(?)

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

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

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

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

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

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

Yours,
Linus Walleij

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

[Sebastian Reichel]

Error printing happens automatically for memory allocation problems.

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

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

[Linus Walleij]

On Thu, Mar 9, 2023 at 11:50 PM Sebastian Reichel <sre@kernel.org> wrote:

> Error printing happens automatically for memory allocation problems.
>
> Signed-off-by: Sebastian Reichel <sre@kernel.org>

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

Yours,
Linus Walleij

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

[Matti Vaittinen]

On 3/10/23 00:50, Sebastian Reichel wrote:
> Error printing happens automatically for memory allocation problems.
> 
> Signed-off-by: Sebastian Reichel <sre@kernel.org>

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

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

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

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

[PATCHv1 08/11] 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.

Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 drivers/power/supply/generic-adc-battery.c | 65 ++--------------------
 include/linux/power/generic-adc-battery.h  | 18 ------
 2 files changed, 5 insertions(+), 78 deletions(-)
 delete mode 100644 include/linux/power/generic-adc-battery.h

diff --git a/drivers/power/supply/generic-adc-battery.c b/drivers/power/supply/generic-adc-battery.c
index 771e5cfc49c3..fc6fcfda1ef2 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
@@ -309,6 +253,7 @@ static int gab_probe(struct platform_device *pdev)
 	 */
 	psy_desc->properties = properties;
 	psy_desc->num_properties = index;
+	psy_desc->expose_battery_info = true;
 
 	adc_bat->psy = devm_power_supply_register(&pdev->dev, psy_desc, &psy_cfg);
 	if (IS_ERR(adc_bat->psy))
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

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

[Linus Walleij]

On Thu, Mar 9, 2023 at 11:50 PM Sebastian Reichel <sre@kernel.org> 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.
>
> Signed-off-by: Sebastian Reichel <sre@kernel.org>

That's neat.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCHv1 09/11] 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().

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 fc6fcfda1ef2..7bc54566664f 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

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

[Linus Walleij]

On Thu, Mar 9, 2023 at 11:50 PM Sebastian Reichel <sre@kernel.org> wrote:

> Drop mostly useless gab_prop_to_chan() function by directly
> supplying the correct enum value to read_channel().
>
> Signed-off-by: Sebastian Reichel <sre@kernel.org>

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

Yours,
Linus Walleij

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

[Matti Vaittinen]

Hi,

Looks good to me.

On 3/10/23 00:50, Sebastian Reichel wrote:
> Drop mostly useless gab_prop_to_chan() function by directly
> supplying the correct enum value to read_channel().
> 
> Signed-off-by: Sebastian Reichel <sre@kernel.org>

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

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

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

[PATCHv1 10/11] 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.

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 7bc54566664f..436e75d226ed 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 */
@@ -170,6 +171,7 @@ static int gab_probe(struct platform_device *pdev)
 	if (!adc_bat)
 		return -ENOMEM;
 
+	psy_cfg.of_node = pdev->dev.of_node;
 	psy_cfg.drv_data = adc_bat;
 	psy_desc = &adc_bat->psy_desc;
 	psy_desc->name = dev_name(&pdev->dev);
@@ -284,10 +286,17 @@ static int __maybe_unused gab_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(gab_pm_ops, gab_suspend, gab_resume);
 
+static const struct of_device_id gab_match[] = {
+	{ .compatible = "adc-battery" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, gab_match);
+
 static struct platform_driver gab_driver = {
 	.driver		= {
 		.name	= "generic-adc-battery",
 		.pm	= &gab_pm_ops,
+		.of_match_table = gab_match,
 	},
 	.probe		= gab_probe,
 };
-- 
2.39.2

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

[Linus Walleij]

On Thu, Mar 9, 2023 at 11:50 PM Sebastian Reichel <sre@kernel.org> wrote:

> This adds full DT support to the driver. Because of the previous
> changes just adding a compatible value is enough.
>
> Signed-off-by: Sebastian Reichel <sre@kernel.org>

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

Yours,
Linus Walleij

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

[Matti Vaittinen]

On 3/10/23 00:50, Sebastian Reichel wrote:
> This adds full DT support to the driver. Because of the previous
> changes just adding a compatible value is enough.
> 
> Signed-off-by: Sebastian Reichel <sre@kernel.org>

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

Best Regards,
	-- Matti

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

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

[PATCHv1 11/11] 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.

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 436e75d226ed..ac72140dc136 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: [PATCHv1 11/11] power: supply: generic-adc-battery: update copyright info

[Linus Walleij]

On Thu, Mar 9, 2023 at 11:50 PM Sebastian Reichel <sre@kernel.org> wrote:

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

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

Yours,
Linus Walleij

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

[Matti Vaittinen]

On 3/10/23 00:50, Sebastian Reichel wrote:
> 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.
> 
> Signed-off-by: Sebastian Reichel <sre@kernel.org>

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

> ---
>   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 436e75d226ed..ac72140dc136 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>

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

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

[PATCH] power: supply: charger-manager: Use of_property_read_bool() for boolean properties

[Rob Herring]

It is preferred to use typed property access functions (i.e.
of_property_read_<type> functions) rather than low-level
of_get_property/of_find_property functions for reading properties.
Convert reading boolean properties to to of_property_read_bool().

Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/power/supply/charger-manager.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/supply/charger-manager.c b/drivers/power/supply/charger-manager.c
index c9e8450c646f..5fa6ba7f41e1 100644
--- a/drivers/power/supply/charger-manager.c
+++ b/drivers/power/supply/charger-manager.c
@@ -1331,7 +1331,7 @@ static struct charger_desc *of_cm_parse_desc(struct device *dev)
 	of_property_read_string(np, "cm-thermal-zone", &desc->thermal_zone);
 
 	of_property_read_u32(np, "cm-battery-cold", &desc->temp_min);
-	if (of_get_property(np, "cm-battery-cold-in-minus", NULL))
+	if (of_property_read_bool(np, "cm-battery-cold-in-minus"))
 		desc->temp_min *= -1;
 	of_property_read_u32(np, "cm-battery-hot", &desc->temp_max);
 	of_property_read_u32(np, "cm-battery-temp-diff", &desc->temp_diff);
-- 
2.39.2