[PATCH v1 0/4] Add optional properties to MAX17040

[PATCH v1 0/4] Add optional properties to MAX17040

[Svyatoslav Ryhel]

Extend properties supported by max17040 fuel gauge with static
simple battery cell properties, some supplier properties (like
health and status) and thermal data from external sensor.

Svyatoslav Ryhel (4):
  dt-bindings: power: supply: maxim,max17040: update properties
  power: max17040: add simple battery cell support
  power: max17040: add passing props from supplier
  power: max17040: get thermal data from adc if available

 .../bindings/power/supply/maxim,max17040.yaml | 37 +++++++++++
 drivers/power/supply/max17040_battery.c       | 66 +++++++++++++++++++
 2 files changed, 103 insertions(+)

-- 
2.37.2

[PATCH v1 1/4] dt-bindings: power: supply: maxim,max17040: update properties

[Svyatoslav Ryhel]

Add simple cell, status, health and temperature properties.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 .../bindings/power/supply/maxim,max17040.yaml | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml b/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml
index 3a529326ecbd..6f1c25b4729f 100644
--- a/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml
+++ b/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml
@@ -55,6 +55,20 @@ properties:
   interrupts:
     maxItems: 1
 
+  monitored-battery:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle to the battery node being monitored
+
+  power-supplies: true
+
+  io-channels:
+    items:
+      - description: battery temperature
+
+  io-channel-names:
+    items:
+      - const: temp
+
   wakeup-source:
     type: boolean
     description: |
@@ -95,3 +109,26 @@ examples:
         wakeup-source;
       };
     };
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c0 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      fuel-gauge@36 {
+        compatible = "maxim,max17043";
+        reg = <0x36>;
+
+        interrupt-parent = <&gpio>;
+        interrupts = <144 IRQ_TYPE_EDGE_FALLING>;
+
+        monitored-battery = <&battery>;
+        power-supplies = <&charger>;
+
+        io-channels = <&adc 8>;
+        io-channel-names = "temp";
+
+        maxim,alert-low-soc-level = <10>;
+        wakeup-source;
+      };
+    };
-- 
2.37.2

[PATCH v1 2/4] power: max17040: add simple battery cell support

[Svyatoslav Ryhel]

Simple battery cell allows to pass some constant data
about battery controlled by this fuel gauge.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/power/supply/max17040_battery.c | 34 +++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
index d1075959dd46..2778ed5b5c14 100644
--- a/drivers/power/supply/max17040_battery.c
+++ b/drivers/power/supply/max17040_battery.c
@@ -141,6 +141,7 @@ struct max17040_chip {
 	struct regmap			*regmap;
 	struct delayed_work		work;
 	struct power_supply		*battery;
+	struct power_supply_battery_info	*batt_info;
 	struct chip_data		data;
 
 	/* battery capacity */
@@ -400,6 +401,28 @@ static int max17040_get_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_CAPACITY_ALERT_MIN:
 		val->intval = chip->low_soc_alert;
 		break;
+
+	case POWER_SUPPLY_PROP_TECHNOLOGY:
+		val->intval = chip->batt_info->technology;
+		break;
+	case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN:
+		val->intval = chip->batt_info->energy_full_design_uwh;
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
+		val->intval = chip->batt_info->charge_full_design_uah;
+		break;
+	case POWER_SUPPLY_PROP_TEMP_MIN:
+		if (chip->batt_info->temp_min == INT_MIN)
+			return -ENODATA;
+
+		val->intval = chip->batt_info->temp_min * 10;
+		break;
+	case POWER_SUPPLY_PROP_TEMP_MAX:
+		if (chip->batt_info->temp_max == INT_MAX)
+			return -ENODATA;
+
+		val->intval = chip->batt_info->temp_max * 10;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -418,6 +441,11 @@ static enum power_supply_property max17040_battery_props[] = {
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
 	POWER_SUPPLY_PROP_CAPACITY,
 	POWER_SUPPLY_PROP_CAPACITY_ALERT_MIN,
+	POWER_SUPPLY_PROP_TECHNOLOGY,
+	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
+	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
+	POWER_SUPPLY_PROP_TEMP_MIN,
+	POWER_SUPPLY_PROP_TEMP_MAX,
 };
 
 static const struct power_supply_desc max17040_battery_desc = {
@@ -470,6 +498,12 @@ static int max17040_probe(struct i2c_client *client)
 		return PTR_ERR(chip->battery);
 	}
 
+	if (client->dev.of_node) {
+		if (power_supply_get_battery_info(chip->battery, &chip->batt_info))
+			dev_warn(&client->dev,
+				 "No monitored battery, some properties will be missing\n");
+	}
+
 	ret = max17040_get_version(chip);
 	if (ret < 0)
 		return ret;
-- 
2.37.2

[PATCH v1 3/4] power: max17040: add passing props from supplier

[Svyatoslav Ryhel]

Optionally pass status and health from supplier if
it supports those props. If cell is online assume it
is present as well.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/power/supply/max17040_battery.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
index 2778ed5b5c14..6dfce7b1309e 100644
--- a/drivers/power/supply/max17040_battery.c
+++ b/drivers/power/supply/max17040_battery.c
@@ -390,6 +390,7 @@ static int max17040_get_property(struct power_supply *psy,
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_ONLINE:
+	case POWER_SUPPLY_PROP_PRESENT:
 		val->intval = max17040_get_online(chip);
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
@@ -402,6 +403,10 @@ static int max17040_get_property(struct power_supply *psy,
 		val->intval = chip->low_soc_alert;
 		break;
 
+	case POWER_SUPPLY_PROP_STATUS:
+	case POWER_SUPPLY_PROP_HEALTH:
+		power_supply_get_property_from_supplier(psy, psp, val);
+		break;
 	case POWER_SUPPLY_PROP_TECHNOLOGY:
 		val->intval = chip->batt_info->technology;
 		break;
@@ -438,10 +443,13 @@ static const struct regmap_config max17040_regmap = {
 
 static enum power_supply_property max17040_battery_props[] = {
 	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
 	POWER_SUPPLY_PROP_CAPACITY,
 	POWER_SUPPLY_PROP_CAPACITY_ALERT_MIN,
 	POWER_SUPPLY_PROP_TECHNOLOGY,
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_HEALTH,
 	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
 	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
 	POWER_SUPPLY_PROP_TEMP_MIN,
-- 
2.37.2

[PATCH v1 4/4] power: max17040: get thermal data from adc if available

[Svyatoslav Ryhel]

Since fuel gauge does not support thermal monitoring,
some vendors may couple this fuel gauge with thermal/adc
sensor to monitor battery cell exact temperature.

Add this feature by adding optional iio thermal channel.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/power/supply/max17040_battery.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
index 6dfce7b1309e..8c743c26dc6e 100644
--- a/drivers/power/supply/max17040_battery.c
+++ b/drivers/power/supply/max17040_battery.c
@@ -18,6 +18,7 @@
 #include <linux/of_device.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
+#include <linux/iio/consumer.h>
 
 #define MAX17040_VCELL	0x02
 #define MAX17040_SOC	0x04
@@ -143,6 +144,7 @@ struct max17040_chip {
 	struct power_supply		*battery;
 	struct power_supply_battery_info	*batt_info;
 	struct chip_data		data;
+	struct iio_channel		*channel_temp;
 
 	/* battery capacity */
 	int soc;
@@ -416,6 +418,11 @@ static int max17040_get_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
 		val->intval = chip->batt_info->charge_full_design_uah;
 		break;
+	case POWER_SUPPLY_PROP_TEMP:
+		iio_read_channel_raw(chip->channel_temp,
+				     &val->intval);
+		val->intval *= 10;
+		break;
 	case POWER_SUPPLY_PROP_TEMP_MIN:
 		if (chip->batt_info->temp_min == INT_MIN)
 			return -ENODATA;
@@ -452,6 +459,7 @@ static enum power_supply_property max17040_battery_props[] = {
 	POWER_SUPPLY_PROP_HEALTH,
 	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
 	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
+	POWER_SUPPLY_PROP_TEMP,
 	POWER_SUPPLY_PROP_TEMP_MIN,
 	POWER_SUPPLY_PROP_TEMP_MAX,
 };
@@ -560,9 +568,24 @@ static int max17040_probe(struct i2c_client *client)
 		}
 	}
 
+	if (of_property_read_bool(client->dev.of_node, "io-channels")) {
+		chip->channel_temp = iio_channel_get(&client->dev, "temp");
+		if (IS_ERR(chip->channel_temp))
+			return dev_err_probe(&client->dev, PTR_ERR(chip->channel_temp),
+					     "failed to get temp\n");
+	};
+
 	return 0;
 }
 
+static void max17040_remove(struct i2c_client *client)
+{
+	struct max17040_chip *chip = i2c_get_clientdata(client);
+
+	if (chip->channel_temp)
+		iio_channel_release(chip->channel_temp);
+}
+
 #ifdef CONFIG_PM_SLEEP
 
 static int max17040_suspend(struct device *dev)
@@ -642,6 +665,7 @@ static struct i2c_driver max17040_i2c_driver = {
 		.pm	= MAX17040_PM_OPS,
 	},
 	.probe_new	= max17040_probe,
+	.remove		= max17040_remove,
 	.id_table	= max17040_id,
 };
 module_i2c_driver(max17040_i2c_driver);
-- 
2.37.2

Re: [PATCH v1 1/4] dt-bindings: power: supply: maxim,max17040: update properties

[Krzysztof Kozlowski]

On 08/03/2023 09:44, Svyatoslav Ryhel wrote:
> Add simple cell, status, health and temperature properties.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  .../bindings/power/supply/maxim,max17040.yaml | 37 +++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml b/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml
> index 3a529326ecbd..6f1c25b4729f 100644
> --- a/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml
> +++ b/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml
> @@ -55,6 +55,20 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> +  monitored-battery:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: phandle to the battery node being monitored
> +
> +  power-supplies: true

This should be rather specific input name, e.g. vdd-supply.

> +
> +  io-channels:
> +    items:
> +      - description: battery temperature



max17040 does not have ADC temperature input... so is it system
configuration?


> +
> +  io-channel-names:
> +    items:
> +      - const: temp

Drop the names property, not needed for one item.

> +
>    wakeup-source:
>      type: boolean
>      description: |
> @@ -95,3 +109,26 @@ examples:
>          wakeup-source;
>        };
>      };
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c0 {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      fuel-gauge@36 {
> +        compatible = "maxim,max17043";
> +        reg = <0x36>;
> +
> +        interrupt-parent = <&gpio>;
> +        interrupts = <144 IRQ_TYPE_EDGE_FALLING>;
> +
> +        monitored-battery = <&battery>;
> +        power-supplies = <&charger>;

But here you suggests something else than VDD... The hardware does not
take charger as input. It takes power supply - vdd.

> +
> +        io-channels = <&adc 8>;

Just add these to existing example.

> +        io-channel-names = "temp";
> +
> +        maxim,alert-low-soc-level = <10>;
> +        wakeup-source;
> +      };
> +    };

Best regards,
Krzysztof

Re: [PATCH v1 4/4] power: max17040: get thermal data from adc if available

[Krzysztof Kozlowski]

On 08/03/2023 09:44, Svyatoslav Ryhel wrote:
> Since fuel gauge does not support thermal monitoring,
> some vendors may couple this fuel gauge with thermal/adc
> sensor to monitor battery cell exact temperature.
> 
> Add this feature by adding optional iio thermal channel.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  drivers/power/supply/max17040_battery.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
> index 6dfce7b1309e..8c743c26dc6e 100644
> --- a/drivers/power/supply/max17040_battery.c
> +++ b/drivers/power/supply/max17040_battery.c
> @@ -18,6 +18,7 @@
>  #include <linux/of_device.h>
>  #include <linux/regmap.h>
>  #include <linux/slab.h>
> +#include <linux/iio/consumer.h>
>  
>  #define MAX17040_VCELL	0x02
>  #define MAX17040_SOC	0x04
> @@ -143,6 +144,7 @@ struct max17040_chip {
>  	struct power_supply		*battery;
>  	struct power_supply_battery_info	*batt_info;
>  	struct chip_data		data;
> +	struct iio_channel		*channel_temp;
>  
>  	/* battery capacity */
>  	int soc;
> @@ -416,6 +418,11 @@ static int max17040_get_property(struct power_supply *psy,
>  	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
>  		val->intval = chip->batt_info->charge_full_design_uah;
>  		break;
> +	case POWER_SUPPLY_PROP_TEMP:
> +		iio_read_channel_raw(chip->channel_temp,
> +				     &val->intval);
> +		val->intval *= 10;

I am not convinced this is needed at all. You basically chain two
subsystems only to report to user-space via power supply, but it is
already reported via IIO. I would understand it if you use the value for
something, e.g. control the charger. Here, it's just feeding different
user-space interface. Therefore:
1. IO channels are not a hardware property of the fuel gauge,
2. I have doubts this should be even exposed via power supply interface.


Best regards,
Krzysztof

Re: [PATCH v1 1/4] dt-bindings: power: supply: maxim,max17040: update properties

[Svyatoslav Ryhel]

ср, 8 бер. 2023 р. о 11:04 Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> пише:
>
> On 08/03/2023 09:44, Svyatoslav Ryhel wrote:
> > Add simple cell, status, health and temperature properties.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> >  .../bindings/power/supply/maxim,max17040.yaml | 37 +++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml b/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml
> > index 3a529326ecbd..6f1c25b4729f 100644
> > --- a/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml
> > +++ b/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml
> > @@ -55,6 +55,20 @@ properties:
> >    interrupts:
> >      maxItems: 1
> >
> > +  monitored-battery:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: phandle to the battery node being monitored
> > +
> > +  power-supplies: true
>
> This should be rather specific input name, e.g. vdd-supply.
>

it is not vdd it is actual charger device

> > +
> > +  io-channels:
> > +    items:
> > +      - description: battery temperature
>
>
>
> max17040 does not have ADC temperature input... so is it system
> configuration?
>

yes, I own a device (LG Optimus Vu P895) which uses max17043
coupled with ADC thermal sensor

> > +
> > +  io-channel-names:
> > +    items:
> > +      - const: temp
>
> Drop the names property, not needed for one item.
>

Alright, but driver patch expects temp name. I will look if this
is adjustable.

> > +
> >    wakeup-source:
> >      type: boolean
> >      description: |
> > @@ -95,3 +109,26 @@ examples:
> >          wakeup-source;
> >        };
> >      };
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    i2c0 {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      fuel-gauge@36 {
> > +        compatible = "maxim,max17043";
> > +        reg = <0x36>;
> > +
> > +        interrupt-parent = <&gpio>;
> > +        interrupts = <144 IRQ_TYPE_EDGE_FALLING>;
> > +
> > +        monitored-battery = <&battery>;
> > +        power-supplies = <&charger>;
>
> But here you suggests something else than VDD... The hardware does not
> take charger as input. It takes power supply - vdd.
>

Power system allows passing properties from other power devices.
In this case battery health and status are passed from charger.

> > +
> > +        io-channels = <&adc 8>;
>
> Just add these to existing example.
>

Not sure if it is a good idea, as you wish.

> > +        io-channel-names = "temp";
> > +
> > +        maxim,alert-low-soc-level = <10>;
> > +        wakeup-source;
> > +      };
> > +    };
>
> Best regards,
> Krzysztof
>

Best regards,
Svyatoslav R.

Re: [PATCH v1 4/4] power: max17040: get thermal data from adc if available

[Svyatoslav Ryhel]

ср, 8 бер. 2023 р. о 11:08 Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> пише:
>
> On 08/03/2023 09:44, Svyatoslav Ryhel wrote:
> > Since fuel gauge does not support thermal monitoring,
> > some vendors may couple this fuel gauge with thermal/adc
> > sensor to monitor battery cell exact temperature.
> >
> > Add this feature by adding optional iio thermal channel.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> >  drivers/power/supply/max17040_battery.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> >
> > diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
> > index 6dfce7b1309e..8c743c26dc6e 100644
> > --- a/drivers/power/supply/max17040_battery.c
> > +++ b/drivers/power/supply/max17040_battery.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/of_device.h>
> >  #include <linux/regmap.h>
> >  #include <linux/slab.h>
> > +#include <linux/iio/consumer.h>
> >
> >  #define MAX17040_VCELL       0x02
> >  #define MAX17040_SOC 0x04
> > @@ -143,6 +144,7 @@ struct max17040_chip {
> >       struct power_supply             *battery;
> >       struct power_supply_battery_info        *batt_info;
> >       struct chip_data                data;
> > +     struct iio_channel              *channel_temp;
> >
> >       /* battery capacity */
> >       int soc;
> > @@ -416,6 +418,11 @@ static int max17040_get_property(struct power_supply *psy,
> >       case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> >               val->intval = chip->batt_info->charge_full_design_uah;
> >               break;
> > +     case POWER_SUPPLY_PROP_TEMP:
> > +             iio_read_channel_raw(chip->channel_temp,
> > +                                  &val->intval);
> > +             val->intval *= 10;
>
> I am not convinced this is needed at all. You basically chain two
> subsystems only to report to user-space via power supply, but it is
> already reported via IIO. I would understand it if you use the value for
> something, e.g. control the charger. Here, it's just feeding different
> user-space interface. Therefore:
> 1. IO channels are not a hardware property of the fuel gauge,
> 2. I have doubts this should be even exposed via power supply interface.
>

I can assure you that this is only the beginning of weird vendor solutions
I have discovered. Nonetheless, max17040 has no battery temp property,
this means in case I have a critical battery overheating, userspace
will tell me nothing
since instead of having direct battery temp property under power supply I have
separate iio sensor, which may not even be monitored. It is always nice to have
battery explosions.

>
> Best regards,
> Krzysztof
>

Re: [PATCH v1 1/4] dt-bindings: power: supply: maxim,max17040: update properties

[Krzysztof Kozlowski]

On 08/03/2023 10:15, Svyatoslav Ryhel wrote:

>> max17040 does not have ADC temperature input... so is it system
>> configuration?
>>
> 
> yes, I own a device (LG Optimus Vu P895) which uses max17043
> coupled with ADC thermal sensor
> 
>>> +
>>> +  io-channel-names:
>>> +    items:
>>> +      - const: temp
>>
>> Drop the names property, not needed for one item.
>>
> 
> Alright, but driver patch expects temp name. I will look if this
> is adjustable.

I think I saw cases without names.

> 
>>> +
>>>    wakeup-source:
>>>      type: boolean
>>>      description: |
>>> @@ -95,3 +109,26 @@ examples:
>>>          wakeup-source;
>>>        };
>>>      };
>>> +  - |
>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>> +    i2c0 {
>>> +      #address-cells = <1>;
>>> +      #size-cells = <0>;
>>> +
>>> +      fuel-gauge@36 {
>>> +        compatible = "maxim,max17043";
>>> +        reg = <0x36>;
>>> +
>>> +        interrupt-parent = <&gpio>;
>>> +        interrupts = <144 IRQ_TYPE_EDGE_FALLING>;
>>> +
>>> +        monitored-battery = <&battery>;
>>> +        power-supplies = <&charger>;
>>
>> But here you suggests something else than VDD... The hardware does not
>> take charger as input. It takes power supply - vdd.
>>
> 
> Power system allows passing properties from other power devices.
> In this case battery health and status are passed from charger.

So this is not an input to device? Then it does not really look like
property of this hardware. Fuel gauge does not control the charger, also
from system configuration point of view.

Best regards,
Krzysztof

Re: [PATCH v1 4/4] power: max17040: get thermal data from adc if available

[Krzysztof Kozlowski]

On 08/03/2023 10:23, Svyatoslav Ryhel wrote:
> ср, 8 бер. 2023 р. о 11:08 Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> пише:
>>
>> On 08/03/2023 09:44, Svyatoslav Ryhel wrote:
>>> Since fuel gauge does not support thermal monitoring,
>>> some vendors may couple this fuel gauge with thermal/adc
>>> sensor to monitor battery cell exact temperature.
>>>
>>> Add this feature by adding optional iio thermal channel.
>>>
>>> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
>>> ---
>>>  drivers/power/supply/max17040_battery.c | 24 ++++++++++++++++++++++++
>>>  1 file changed, 24 insertions(+)
>>>
>>> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
>>> index 6dfce7b1309e..8c743c26dc6e 100644
>>> --- a/drivers/power/supply/max17040_battery.c
>>> +++ b/drivers/power/supply/max17040_battery.c
>>> @@ -18,6 +18,7 @@
>>>  #include <linux/of_device.h>
>>>  #include <linux/regmap.h>
>>>  #include <linux/slab.h>
>>> +#include <linux/iio/consumer.h>
>>>
>>>  #define MAX17040_VCELL       0x02
>>>  #define MAX17040_SOC 0x04
>>> @@ -143,6 +144,7 @@ struct max17040_chip {
>>>       struct power_supply             *battery;
>>>       struct power_supply_battery_info        *batt_info;
>>>       struct chip_data                data;
>>> +     struct iio_channel              *channel_temp;
>>>
>>>       /* battery capacity */
>>>       int soc;
>>> @@ -416,6 +418,11 @@ static int max17040_get_property(struct power_supply *psy,
>>>       case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
>>>               val->intval = chip->batt_info->charge_full_design_uah;
>>>               break;
>>> +     case POWER_SUPPLY_PROP_TEMP:
>>> +             iio_read_channel_raw(chip->channel_temp,
>>> +                                  &val->intval);
>>> +             val->intval *= 10;
>>
>> I am not convinced this is needed at all. You basically chain two
>> subsystems only to report to user-space via power supply, but it is
>> already reported via IIO. I would understand it if you use the value for
>> something, e.g. control the charger. Here, it's just feeding different
>> user-space interface. Therefore:
>> 1. IO channels are not a hardware property of the fuel gauge,
>> 2. I have doubts this should be even exposed via power supply interface.
>>
> 
> I can assure you that this is only the beginning of weird vendor solutions
> I have discovered. Nonetheless, max17040 has no battery temp property,
> this means in case I have a critical battery overheating, userspace
> will tell me nothing

Of course will tell you - via IIO.

> since instead of having direct battery temp property under power supply I have
> separate iio sensor, which may not even be monitored. It is always nice to have
> battery explosions.

Hm, ok, I defer this to Sebastian. What's the policy - who should report
battery temperature if the battery/FG itself does not have any sensor?

Best regards,
Krzysztof

Re: [PATCH v1 1/4] dt-bindings: power: supply: maxim,max17040: update properties

[Svyatoslav Ryhel]

ср, 8 бер. 2023 р. о 12:44 Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> пише:
>
> On 08/03/2023 10:15, Svyatoslav Ryhel wrote:
>
> >> max17040 does not have ADC temperature input... so is it system
> >> configuration?
> >>
> >
> > yes, I own a device (LG Optimus Vu P895) which uses max17043
> > coupled with ADC thermal sensor
> >
> >>> +
> >>> +  io-channel-names:
> >>> +    items:
> >>> +      - const: temp
> >>
> >> Drop the names property, not needed for one item.
> >>
> >
> > Alright, but driver patch expects temp name. I will look if this
> > is adjustable.
>
> I think I saw cases without names.
>

There is no io-channel without a name. And io-channels are mostly used
by power supply devices.

> >
> >>> +
> >>>    wakeup-source:
> >>>      type: boolean
> >>>      description: |
> >>> @@ -95,3 +109,26 @@ examples:
> >>>          wakeup-source;
> >>>        };
> >>>      };
> >>> +  - |
> >>> +    #include <dt-bindings/interrupt-controller/irq.h>
> >>> +    i2c0 {
> >>> +      #address-cells = <1>;
> >>> +      #size-cells = <0>;
> >>> +
> >>> +      fuel-gauge@36 {
> >>> +        compatible = "maxim,max17043";
> >>> +        reg = <0x36>;
> >>> +
> >>> +        interrupt-parent = <&gpio>;
> >>> +        interrupts = <144 IRQ_TYPE_EDGE_FALLING>;
> >>> +
> >>> +        monitored-battery = <&battery>;
> >>> +        power-supplies = <&charger>;
> >>
> >> But here you suggests something else than VDD... The hardware does not
> >> take charger as input. It takes power supply - vdd.
> >>
> >
> > Power system allows passing properties from other power devices.
> > In this case battery health and status are passed from charger.
>
> So this is not an input to device? Then it does not really look like
> property of this hardware. Fuel gauge does not control the charger, also
> from system configuration point of view.
>

It is not controlling charger, the charger provides the status and
health of the battery to the fuel gauge. This option is also used in
other fuel gauges.

> Best regards,
> Krzysztof
>

Re: [PATCH v1 1/4] dt-bindings: power: supply: maxim,max17040: update properties

[Krzysztof Kozlowski]

On 08/03/2023 11:51, Svyatoslav Ryhel wrote:
> ср, 8 бер. 2023 р. о 12:44 Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> пише:
>>
>> On 08/03/2023 10:15, Svyatoslav Ryhel wrote:
>>
>>>> max17040 does not have ADC temperature input... so is it system
>>>> configuration?
>>>>
>>>
>>> yes, I own a device (LG Optimus Vu P895) which uses max17043
>>> coupled with ADC thermal sensor
>>>
>>>>> +
>>>>> +  io-channel-names:
>>>>> +    items:
>>>>> +      - const: temp
>>>>
>>>> Drop the names property, not needed for one item.
>>>>
>>>
>>> Alright, but driver patch expects temp name. I will look if this
>>> is adjustable.
>>
>> I think I saw cases without names.
>>
> 
> There is no io-channel without a name. And io-channels are mostly used
> by power supply devices.
> 
>>>
>>>>> +
>>>>>    wakeup-source:
>>>>>      type: boolean
>>>>>      description: |
>>>>> @@ -95,3 +109,26 @@ examples:
>>>>>          wakeup-source;
>>>>>        };
>>>>>      };
>>>>> +  - |
>>>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>>>> +    i2c0 {
>>>>> +      #address-cells = <1>;
>>>>> +      #size-cells = <0>;
>>>>> +
>>>>> +      fuel-gauge@36 {
>>>>> +        compatible = "maxim,max17043";
>>>>> +        reg = <0x36>;
>>>>> +
>>>>> +        interrupt-parent = <&gpio>;
>>>>> +        interrupts = <144 IRQ_TYPE_EDGE_FALLING>;
>>>>> +
>>>>> +        monitored-battery = <&battery>;
>>>>> +        power-supplies = <&charger>;
>>>>
>>>> But here you suggests something else than VDD... The hardware does not
>>>> take charger as input. It takes power supply - vdd.
>>>>
>>>
>>> Power system allows passing properties from other power devices.
>>> In this case battery health and status are passed from charger.
>>
>> So this is not an input to device? Then it does not really look like
>> property of this hardware. Fuel gauge does not control the charger, also
>> from system configuration point of view.
>>
> 
> It is not controlling charger, the charger provides the status and
> health of the battery to the fuel gauge. This option is also used in
> other fuel gauges.

How regulator provides health and status of the battery? I don't understand.

Best regards,
Krzysztof

Re: [PATCH v1 1/4] dt-bindings: power: supply: maxim,max17040: update properties

[Svyatoslav Ryhel]

ср, 8 бер. 2023 р. о 12:53 Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> пише:
>
> On 08/03/2023 11:51, Svyatoslav Ryhel wrote:
> > ср, 8 бер. 2023 р. о 12:44 Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> пише:
> >>
> >> On 08/03/2023 10:15, Svyatoslav Ryhel wrote:
> >>
> >>>> max17040 does not have ADC temperature input... so is it system
> >>>> configuration?
> >>>>
> >>>
> >>> yes, I own a device (LG Optimus Vu P895) which uses max17043
> >>> coupled with ADC thermal sensor
> >>>
> >>>>> +
> >>>>> +  io-channel-names:
> >>>>> +    items:
> >>>>> +      - const: temp
> >>>>
> >>>> Drop the names property, not needed for one item.
> >>>>
> >>>
> >>> Alright, but driver patch expects temp name. I will look if this
> >>> is adjustable.
> >>
> >> I think I saw cases without names.
> >>
> >
> > There is no io-channel without a name. And io-channels are mostly used
> > by power supply devices.
> >
> >>>
> >>>>> +
> >>>>>    wakeup-source:
> >>>>>      type: boolean
> >>>>>      description: |
> >>>>> @@ -95,3 +109,26 @@ examples:
> >>>>>          wakeup-source;
> >>>>>        };
> >>>>>      };
> >>>>> +  - |
> >>>>> +    #include <dt-bindings/interrupt-controller/irq.h>
> >>>>> +    i2c0 {
> >>>>> +      #address-cells = <1>;
> >>>>> +      #size-cells = <0>;
> >>>>> +
> >>>>> +      fuel-gauge@36 {
> >>>>> +        compatible = "maxim,max17043";
> >>>>> +        reg = <0x36>;
> >>>>> +
> >>>>> +        interrupt-parent = <&gpio>;
> >>>>> +        interrupts = <144 IRQ_TYPE_EDGE_FALLING>;
> >>>>> +
> >>>>> +        monitored-battery = <&battery>;
> >>>>> +        power-supplies = <&charger>;
> >>>>
> >>>> But here you suggests something else than VDD... The hardware does not
> >>>> take charger as input. It takes power supply - vdd.
> >>>>
> >>>
> >>> Power system allows passing properties from other power devices.
> >>> In this case battery health and status are passed from charger.
> >>
> >> So this is not an input to device? Then it does not really look like
> >> property of this hardware. Fuel gauge does not control the charger, also
> >> from system configuration point of view.
> >>
> >
> > It is not controlling charger, the charger provides the status and
> > health of the battery to the fuel gauge. This option is also used in
> > other fuel gauges.
>
> How regulator provides health and status of the battery? I don't understand.
>

It is not a regulator, it is a charger! Dedicated chip responsible for
controlling charging. And its configuration allows it to get battery
health and status, because this fuel gauge does not have this
function.

> Best regards,
> Krzysztof
>

Re: [PATCH v1 1/4] dt-bindings: power: supply: maxim,max17040: update properties

[Krzysztof Kozlowski]

On 08/03/2023 12:06, Svyatoslav Ryhel wrote:
> ср, 8 бер. 2023 р. о 12:53 Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> пише:
>>
>> On 08/03/2023 11:51, Svyatoslav Ryhel wrote:
>>> ср, 8 бер. 2023 р. о 12:44 Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> пише:
>>>>
>>>> On 08/03/2023 10:15, Svyatoslav Ryhel wrote:
>>>>
>>>>>> max17040 does not have ADC temperature input... so is it system
>>>>>> configuration?
>>>>>>
>>>>>
>>>>> yes, I own a device (LG Optimus Vu P895) which uses max17043
>>>>> coupled with ADC thermal sensor
>>>>>
>>>>>>> +
>>>>>>> +  io-channel-names:
>>>>>>> +    items:
>>>>>>> +      - const: temp
>>>>>>
>>>>>> Drop the names property, not needed for one item.
>>>>>>
>>>>>
>>>>> Alright, but driver patch expects temp name. I will look if this
>>>>> is adjustable.
>>>>
>>>> I think I saw cases without names.
>>>>
>>>
>>> There is no io-channel without a name. And io-channels are mostly used
>>> by power supply devices.
>>>
>>>>>
>>>>>>> +
>>>>>>>    wakeup-source:
>>>>>>>      type: boolean
>>>>>>>      description: |
>>>>>>> @@ -95,3 +109,26 @@ examples:
>>>>>>>          wakeup-source;
>>>>>>>        };
>>>>>>>      };
>>>>>>> +  - |
>>>>>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>>>>>> +    i2c0 {
>>>>>>> +      #address-cells = <1>;
>>>>>>> +      #size-cells = <0>;
>>>>>>> +
>>>>>>> +      fuel-gauge@36 {
>>>>>>> +        compatible = "maxim,max17043";
>>>>>>> +        reg = <0x36>;
>>>>>>> +
>>>>>>> +        interrupt-parent = <&gpio>;
>>>>>>> +        interrupts = <144 IRQ_TYPE_EDGE_FALLING>;
>>>>>>> +
>>>>>>> +        monitored-battery = <&battery>;
>>>>>>> +        power-supplies = <&charger>;
>>>>>>
>>>>>> But here you suggests something else than VDD... The hardware does not
>>>>>> take charger as input. It takes power supply - vdd.
>>>>>>
>>>>>
>>>>> Power system allows passing properties from other power devices.
>>>>> In this case battery health and status are passed from charger.
>>>>
>>>> So this is not an input to device? Then it does not really look like
>>>> property of this hardware. Fuel gauge does not control the charger, also
>>>> from system configuration point of view.
>>>>
>>>
>>> It is not controlling charger, the charger provides the status and
>>> health of the battery to the fuel gauge. This option is also used in
>>> other fuel gauges.
>>
>> How regulator provides health and status of the battery? I don't understand.
>>
> 
> It is not a regulator, it is a charger! Dedicated chip responsible for
> controlling charging. And its configuration allows it to get battery
> health and status, because this fuel gauge does not have this
> function.


Ah, you are right. I confused with power-supply. It is fine.


Best regards,
Krzysztof

Re: [PATCH v1 4/4] power: max17040: get thermal data from adc if available

[kernel test robot]

Hi Svyatoslav,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on sre-power-supply/for-next]
[also build test ERROR on krzk-dt/for-next linus/master v6.3-rc1 next-20230308]
[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/Svyatoslav-Ryhel/dt-bindings-power-supply-maxim-max17040-update-properties/20230308-164538
base:   https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git for-next
patch link:    https://lore.kernel.org/r/20230308084419.11934-5-clamor95%40gmail.com
patch subject: [PATCH v1 4/4] power: max17040: get thermal data from adc if available
config: xtensa-randconfig-r002-20230308 (https://download.01.org/0day-ci/archive/20230309/202303090802.G5XRtUbY-lkp@intel.com/config)
compiler: xtensa-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/7b9bbf6f2b910ef4ffab18022223573e9094f007
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Svyatoslav-Ryhel/dt-bindings-power-supply-maxim-max17040-update-properties/20230308-164538
        git checkout 7b9bbf6f2b910ef4ffab18022223573e9094f007
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=xtensa olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=xtensa 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/202303090802.G5XRtUbY-lkp@intel.com/

All errors (new ones prefixed by >>):

   arch/xtensa/kernel/entry.o: in function `fast_syscall_spill_registers':
   arch/xtensa/kernel/entry.S:1424:(.exception.text+0x1e3): dangerous relocation: windowed longcall crosses 1GB boundary; return may fail: make_task_dead
   xtensa-linux-ld: drivers/power/supply/max17040_battery.o: in function `max17040_stop_work':
>> max17040_battery.c:(.text+0x60): undefined reference to `iio_read_channel_raw'
   xtensa-linux-ld: drivers/power/supply/max17040_battery.o: in function `max17040_get_property':
   max17040_battery.c:(.text+0x11e): undefined reference to `iio_read_channel_raw'
>> xtensa-linux-ld: max17040_battery.c:(.text+0x170): undefined reference to `iio_channel_release'
   xtensa-linux-ld: drivers/power/supply/max17040_battery.o: in function `max17040_remove':
>> max17040_battery.c:(.text+0x188): undefined reference to `iio_channel_release'
>> xtensa-linux-ld: max17040_battery.c:(.text+0x260): undefined reference to `iio_channel_get'
   xtensa-linux-ld: drivers/power/supply/max17040_battery.o: in function `max17040_probe':
>> max17040_battery.c:(.text+0x542): undefined reference to `iio_channel_get'

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

Re: [PATCH v1 4/4] power: max17040: get thermal data from adc if available

[kernel test robot]

Hi Svyatoslav,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on sre-power-supply/for-next]
[also build test ERROR on krzk-dt/for-next linus/master v6.3-rc1 next-20230308]
[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/Svyatoslav-Ryhel/dt-bindings-power-supply-maxim-max17040-update-properties/20230308-164538
base:   https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git for-next
patch link:    https://lore.kernel.org/r/20230308084419.11934-5-clamor95%40gmail.com
patch subject: [PATCH v1 4/4] power: max17040: get thermal data from adc if available
config: hexagon-randconfig-r035-20230308 (https://download.01.org/0day-ci/archive/20230309/202303090817.SuiCpxvy-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/7b9bbf6f2b910ef4ffab18022223573e9094f007
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Svyatoslav-Ryhel/dt-bindings-power-supply-maxim-max17040-update-properties/20230308-164538
        git checkout 7b9bbf6f2b910ef4ffab18022223573e9094f007
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon 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/202303090817.SuiCpxvy-lkp@intel.com/

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: iio_channel_release
   >>> referenced by max17040_battery.c:586 (drivers/power/supply/max17040_battery.c:586)
   >>>               drivers/power/supply/max17040_battery.o:(max17040_remove) in archive vmlinux.a
   >>> referenced by max17040_battery.c:586 (drivers/power/supply/max17040_battery.c:586)
   >>>               drivers/power/supply/max17040_battery.o:(max17040_remove) in archive vmlinux.a
--
>> ld.lld: error: undefined symbol: iio_channel_get
   >>> referenced by max17040_battery.c:572 (drivers/power/supply/max17040_battery.c:572)
   >>>               drivers/power/supply/max17040_battery.o:(max17040_probe) in archive vmlinux.a
   >>> referenced by max17040_battery.c:572 (drivers/power/supply/max17040_battery.c:572)
   >>>               drivers/power/supply/max17040_battery.o:(max17040_probe) in archive vmlinux.a
--
>> ld.lld: error: undefined symbol: iio_read_channel_raw
   >>> referenced by max17040_battery.c:422 (drivers/power/supply/max17040_battery.c:422)
   >>>               drivers/power/supply/max17040_battery.o:(max17040_get_property) in archive vmlinux.a
   >>> referenced by max17040_battery.c:422 (drivers/power/supply/max17040_battery.c:422)
   >>>               drivers/power/supply/max17040_battery.o:(max17040_get_property) in archive vmlinux.a

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

Re: [PATCH v1 4/4] power: max17040: get thermal data from adc if available

[kernel test robot]

Hi Svyatoslav,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on sre-power-supply/for-next]
[also build test ERROR on krzk-dt/for-next linus/master v6.3-rc1 next-20230308]
[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/Svyatoslav-Ryhel/dt-bindings-power-supply-maxim-max17040-update-properties/20230308-164538
base:   https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git for-next
patch link:    https://lore.kernel.org/r/20230308084419.11934-5-clamor95%40gmail.com
patch subject: [PATCH v1 4/4] power: max17040: get thermal data from adc if available
config: csky-randconfig-r015-20230308 (https://download.01.org/0day-ci/archive/20230309/202303090836.dpbjGxDu-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/7b9bbf6f2b910ef4ffab18022223573e9094f007
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Svyatoslav-Ryhel/dt-bindings-power-supply-maxim-max17040-update-properties/20230308-164538
        git checkout 7b9bbf6f2b910ef4ffab18022223573e9094f007
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=csky olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=csky 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/202303090836.dpbjGxDu-lkp@intel.com/

All errors (new ones prefixed by >>):

   csky-linux-ld: drivers/power/supply/max17040_battery.o: in function `max17040_get_property':
>> drivers/power/supply/max17040_battery.c:422: undefined reference to `iio_read_channel_raw'
   csky-linux-ld: drivers/power/supply/max17040_battery.o: in function `max17040_remove':
>> drivers/power/supply/max17040_battery.c:586: undefined reference to `iio_channel_release'
>> csky-linux-ld: drivers/power/supply/max17040_battery.c:587: undefined reference to `iio_read_channel_raw'
>> csky-linux-ld: drivers/power/supply/max17040_battery.c:587: undefined reference to `iio_channel_release'
   csky-linux-ld: drivers/power/supply/max17040_battery.o: in function `max17040_probe':
>> drivers/power/supply/max17040_battery.c:572: undefined reference to `iio_channel_get'
   csky-linux-ld: drivers/power/supply/max17040_battery.o: in function `max17040_work':
   drivers/power/supply/max17040_battery.c:297: undefined reference to `iio_channel_get'
   pahole: .tmp_vmlinux.btf: Invalid argument
   .btf.vmlinux.bin.o: file not recognized: file format not recognized


vim +422 drivers/power/supply/max17040_battery.c

   386	
   387	static int max17040_get_property(struct power_supply *psy,
   388				    enum power_supply_property psp,
   389				    union power_supply_propval *val)
   390	{
   391		struct max17040_chip *chip = power_supply_get_drvdata(psy);
   392	
   393		switch (psp) {
   394		case POWER_SUPPLY_PROP_ONLINE:
   395		case POWER_SUPPLY_PROP_PRESENT:
   396			val->intval = max17040_get_online(chip);
   397			break;
   398		case POWER_SUPPLY_PROP_VOLTAGE_NOW:
   399			val->intval = max17040_get_vcell(chip);
   400			break;
   401		case POWER_SUPPLY_PROP_CAPACITY:
   402			val->intval = max17040_get_soc(chip);
   403			break;
   404		case POWER_SUPPLY_PROP_CAPACITY_ALERT_MIN:
   405			val->intval = chip->low_soc_alert;
   406			break;
   407	
   408		case POWER_SUPPLY_PROP_STATUS:
   409		case POWER_SUPPLY_PROP_HEALTH:
   410			power_supply_get_property_from_supplier(psy, psp, val);
   411			break;
   412		case POWER_SUPPLY_PROP_TECHNOLOGY:
   413			val->intval = chip->batt_info->technology;
   414			break;
   415		case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN:
   416			val->intval = chip->batt_info->energy_full_design_uwh;
   417			break;
   418		case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
   419			val->intval = chip->batt_info->charge_full_design_uah;
   420			break;
   421		case POWER_SUPPLY_PROP_TEMP:
 > 422			iio_read_channel_raw(chip->channel_temp,
   423					     &val->intval);
   424			val->intval *= 10;
   425			break;
   426		case POWER_SUPPLY_PROP_TEMP_MIN:
   427			if (chip->batt_info->temp_min == INT_MIN)
   428				return -ENODATA;
   429	
   430			val->intval = chip->batt_info->temp_min * 10;
   431			break;
   432		case POWER_SUPPLY_PROP_TEMP_MAX:
   433			if (chip->batt_info->temp_max == INT_MAX)
   434				return -ENODATA;
   435	
   436			val->intval = chip->batt_info->temp_max * 10;
   437			break;
   438		default:
   439			return -EINVAL;
   440		}
   441		return 0;
   442	}
   443	
   444	static const struct regmap_config max17040_regmap = {
   445		.reg_bits	= 8,
   446		.reg_stride	= 2,
   447		.val_bits	= 16,
   448		.val_format_endian = REGMAP_ENDIAN_BIG,
   449	};
   450	
   451	static enum power_supply_property max17040_battery_props[] = {
   452		POWER_SUPPLY_PROP_ONLINE,
   453		POWER_SUPPLY_PROP_PRESENT,
   454		POWER_SUPPLY_PROP_VOLTAGE_NOW,
   455		POWER_SUPPLY_PROP_CAPACITY,
   456		POWER_SUPPLY_PROP_CAPACITY_ALERT_MIN,
   457		POWER_SUPPLY_PROP_TECHNOLOGY,
   458		POWER_SUPPLY_PROP_STATUS,
   459		POWER_SUPPLY_PROP_HEALTH,
   460		POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
   461		POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
   462		POWER_SUPPLY_PROP_TEMP,
   463		POWER_SUPPLY_PROP_TEMP_MIN,
   464		POWER_SUPPLY_PROP_TEMP_MAX,
   465	};
   466	
   467	static const struct power_supply_desc max17040_battery_desc = {
   468		.name			= "battery",
   469		.type			= POWER_SUPPLY_TYPE_BATTERY,
   470		.get_property		= max17040_get_property,
   471		.set_property		= max17040_set_property,
   472		.property_is_writeable  = max17040_prop_writeable,
   473		.properties		= max17040_battery_props,
   474		.num_properties		= ARRAY_SIZE(max17040_battery_props),
   475	};
   476	
   477	static int max17040_probe(struct i2c_client *client)
   478	{
   479		const struct i2c_device_id *id = i2c_client_get_device_id(client);
   480		struct i2c_adapter *adapter = client->adapter;
   481		struct power_supply_config psy_cfg = {};
   482		struct max17040_chip *chip;
   483		enum chip_id chip_id;
   484		bool enable_irq = false;
   485		int ret;
   486	
   487		if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
   488			return -EIO;
   489	
   490		chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
   491		if (!chip)
   492			return -ENOMEM;
   493	
   494		chip->client = client;
   495		chip->regmap = devm_regmap_init_i2c(client, &max17040_regmap);
   496		if (IS_ERR(chip->regmap))
   497			return PTR_ERR(chip->regmap);
   498		chip_id = (enum chip_id) id->driver_data;
   499		if (client->dev.of_node) {
   500			ret = max17040_get_of_data(chip);
   501			if (ret)
   502				return ret;
   503			chip_id = (uintptr_t)of_device_get_match_data(&client->dev);
   504		}
   505		chip->data = max17040_family[chip_id];
   506	
   507		i2c_set_clientdata(client, chip);
   508		psy_cfg.drv_data = chip;
   509	
   510		chip->battery = devm_power_supply_register(&client->dev,
   511					&max17040_battery_desc, &psy_cfg);
   512		if (IS_ERR(chip->battery)) {
   513			dev_err(&client->dev, "failed: power supply register\n");
   514			return PTR_ERR(chip->battery);
   515		}
   516	
   517		if (client->dev.of_node) {
   518			if (power_supply_get_battery_info(chip->battery, &chip->batt_info))
   519				dev_warn(&client->dev,
   520					 "No monitored battery, some properties will be missing\n");
   521		}
   522	
   523		ret = max17040_get_version(chip);
   524		if (ret < 0)
   525			return ret;
   526		dev_dbg(&chip->client->dev, "MAX17040 Fuel-Gauge Ver 0x%x\n", ret);
   527	
   528		if (chip_id == ID_MAX17040 || chip_id == ID_MAX17041)
   529			max17040_reset(chip);
   530	
   531		max17040_set_rcomp(chip, chip->rcomp);
   532	
   533		/* check interrupt */
   534		if (client->irq && chip->data.has_low_soc_alert) {
   535			ret = max17040_set_low_soc_alert(chip, chip->low_soc_alert);
   536			if (ret) {
   537				dev_err(&client->dev,
   538					"Failed to set low SOC alert: err %d\n", ret);
   539				return ret;
   540			}
   541	
   542			enable_irq = true;
   543		}
   544	
   545		if (client->irq && chip->data.has_soc_alert) {
   546			ret = max17040_set_soc_alert(chip, 1);
   547			if (ret) {
   548				dev_err(&client->dev,
   549					"Failed to set SOC alert: err %d\n", ret);
   550				return ret;
   551			}
   552			enable_irq = true;
   553		} else {
   554			/* soc alerts negate the need for polling */
   555			INIT_DEFERRABLE_WORK(&chip->work, max17040_work);
   556			ret = devm_add_action(&client->dev, max17040_stop_work, chip);
   557			if (ret)
   558				return ret;
   559			max17040_queue_work(chip);
   560		}
   561	
   562		if (enable_irq) {
   563			ret = max17040_enable_alert_irq(chip);
   564			if (ret) {
   565				client->irq = 0;
   566				dev_warn(&client->dev,
   567					 "Failed to get IRQ err %d\n", ret);
   568			}
   569		}
   570	
   571		if (of_property_read_bool(client->dev.of_node, "io-channels")) {
 > 572			chip->channel_temp = iio_channel_get(&client->dev, "temp");
   573			if (IS_ERR(chip->channel_temp))
   574				return dev_err_probe(&client->dev, PTR_ERR(chip->channel_temp),
   575						     "failed to get temp\n");
   576		};
   577	
   578		return 0;
   579	}
   580	
   581	static void max17040_remove(struct i2c_client *client)
   582	{
   583		struct max17040_chip *chip = i2c_get_clientdata(client);
   584	
   585		if (chip->channel_temp)
 > 586			iio_channel_release(chip->channel_temp);
 > 587	}
   588	

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

Re: [PATCH v1 1/4] dt-bindings: power: supply: maxim,max17040: update properties

[Sebastian Reichel]

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

Hi,

On Wed, Mar 08, 2023 at 10:44:16AM +0200, Svyatoslav Ryhel wrote:
> Add simple cell, status, health and temperature properties.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  .../bindings/power/supply/maxim,max17040.yaml | 37 +++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml b/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml
> index 3a529326ecbd..6f1c25b4729f 100644
> --- a/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml
> +++ b/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml
> @@ -55,6 +55,20 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> +  monitored-battery:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: phandle to the battery node being monitored
> +
> +  power-supplies: true

The above two should not be needed, since the binding inherits them:

```
allOf:
  - $ref: power-supply.yaml#

unevaluatedProperties: false
```

Otherwise LGTM.

-- Sebastian

> +
> +  io-channels:
> +    items:
> +      - description: battery temperature
> +
> +  io-channel-names:
> +    items:
> +      - const: temp
> +
>    wakeup-source:
>      type: boolean
>      description: |
> @@ -95,3 +109,26 @@ examples:
>          wakeup-source;
>        };
>      };
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c0 {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      fuel-gauge@36 {
> +        compatible = "maxim,max17043";
> +        reg = <0x36>;
> +
> +        interrupt-parent = <&gpio>;
> +        interrupts = <144 IRQ_TYPE_EDGE_FALLING>;
> +
> +        monitored-battery = <&battery>;
> +        power-supplies = <&charger>;
> +
> +        io-channels = <&adc 8>;
> +        io-channel-names = "temp";
> +
> +        maxim,alert-low-soc-level = <10>;
> +        wakeup-source;
> +      };
> +    };
> -- 
> 2.37.2
> 

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

Re: [PATCH v1 2/4] power: max17040: add simple battery cell support

[Sebastian Reichel]

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

Hi,

On Wed, Mar 08, 2023 at 10:44:17AM +0200, Svyatoslav Ryhel wrote:
> Simple battery cell allows to pass some constant data
> about battery controlled by this fuel gauge.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---

This patch is no longer needed, since the core should automatically
expose the properties with the latest kernel:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=27a2195efa8d26447c40dd4a6299ea0247786d75

-- Sebastian

>  drivers/power/supply/max17040_battery.c | 34 +++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
> index d1075959dd46..2778ed5b5c14 100644
> --- a/drivers/power/supply/max17040_battery.c
> +++ b/drivers/power/supply/max17040_battery.c
> @@ -141,6 +141,7 @@ struct max17040_chip {
>  	struct regmap			*regmap;
>  	struct delayed_work		work;
>  	struct power_supply		*battery;
> +	struct power_supply_battery_info	*batt_info;
>  	struct chip_data		data;
>  
>  	/* battery capacity */
> @@ -400,6 +401,28 @@ static int max17040_get_property(struct power_supply *psy,
>  	case POWER_SUPPLY_PROP_CAPACITY_ALERT_MIN:
>  		val->intval = chip->low_soc_alert;
>  		break;
> +
> +	case POWER_SUPPLY_PROP_TECHNOLOGY:
> +		val->intval = chip->batt_info->technology;
> +		break;
> +	case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN:
> +		val->intval = chip->batt_info->energy_full_design_uwh;
> +		break;
> +	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> +		val->intval = chip->batt_info->charge_full_design_uah;
> +		break;
> +	case POWER_SUPPLY_PROP_TEMP_MIN:
> +		if (chip->batt_info->temp_min == INT_MIN)
> +			return -ENODATA;
> +
> +		val->intval = chip->batt_info->temp_min * 10;
> +		break;
> +	case POWER_SUPPLY_PROP_TEMP_MAX:
> +		if (chip->batt_info->temp_max == INT_MAX)
> +			return -ENODATA;
> +
> +		val->intval = chip->batt_info->temp_max * 10;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -418,6 +441,11 @@ static enum power_supply_property max17040_battery_props[] = {
>  	POWER_SUPPLY_PROP_VOLTAGE_NOW,
>  	POWER_SUPPLY_PROP_CAPACITY,
>  	POWER_SUPPLY_PROP_CAPACITY_ALERT_MIN,
> +	POWER_SUPPLY_PROP_TECHNOLOGY,
> +	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
> +	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
> +	POWER_SUPPLY_PROP_TEMP_MIN,
> +	POWER_SUPPLY_PROP_TEMP_MAX,
>  };
>  
>  static const struct power_supply_desc max17040_battery_desc = {
> @@ -470,6 +498,12 @@ static int max17040_probe(struct i2c_client *client)
>  		return PTR_ERR(chip->battery);
>  	}
>  
> +	if (client->dev.of_node) {
> +		if (power_supply_get_battery_info(chip->battery, &chip->batt_info))
> +			dev_warn(&client->dev,
> +				 "No monitored battery, some properties will be missing\n");
> +	}
> +
>  	ret = max17040_get_version(chip);
>  	if (ret < 0)
>  		return ret;
> -- 
> 2.37.2
> 

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

Re: [PATCH v1 4/4] power: max17040: get thermal data from adc if available

[Sebastian Reichel]

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

Hi,

On Wed, Mar 08, 2023 at 10:44:19AM +0200, Svyatoslav Ryhel wrote:
> Since fuel gauge does not support thermal monitoring,
> some vendors may couple this fuel gauge with thermal/adc
> sensor to monitor battery cell exact temperature.
> 
> Add this feature by adding optional iio thermal channel.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  drivers/power/supply/max17040_battery.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
> index 6dfce7b1309e..8c743c26dc6e 100644
> --- a/drivers/power/supply/max17040_battery.c
> +++ b/drivers/power/supply/max17040_battery.c
> @@ -18,6 +18,7 @@
>  #include <linux/of_device.h>
>  #include <linux/regmap.h>
>  #include <linux/slab.h>
> +#include <linux/iio/consumer.h>
>  
>  #define MAX17040_VCELL	0x02
>  #define MAX17040_SOC	0x04
> @@ -143,6 +144,7 @@ struct max17040_chip {
>  	struct power_supply		*battery;
>  	struct power_supply_battery_info	*batt_info;
>  	struct chip_data		data;
> +	struct iio_channel		*channel_temp;
>  
>  	/* battery capacity */
>  	int soc;
> @@ -416,6 +418,11 @@ static int max17040_get_property(struct power_supply *psy,
>  	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
>  		val->intval = chip->batt_info->charge_full_design_uah;
>  		break;
> +	case POWER_SUPPLY_PROP_TEMP:
> +		iio_read_channel_raw(chip->channel_temp,
> +				     &val->intval);
> +		val->intval *= 10;

return iio_read_channel_processed_scale(chip->channel_temp, &val->intval, 10);

> +		break;
>  	case POWER_SUPPLY_PROP_TEMP_MIN:
>  		if (chip->batt_info->temp_min == INT_MIN)
>  			return -ENODATA;
> @@ -452,6 +459,7 @@ static enum power_supply_property max17040_battery_props[] = {
>  	POWER_SUPPLY_PROP_HEALTH,
>  	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
>  	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
> +	POWER_SUPPLY_PROP_TEMP,

You should only expose this, if chip->channel_temp is not NULL. Use
devm_kmemdup() to copy the array into a private copy in chip and
modify it on the fly.
    
>  	POWER_SUPPLY_PROP_TEMP_MIN,
>  	POWER_SUPPLY_PROP_TEMP_MAX,
>  };
> @@ -560,9 +568,24 @@ static int max17040_probe(struct i2c_client *client)
>  		}
>  	}
>  
> +	if (of_property_read_bool(client->dev.of_node, "io-channels")) {

device_property_present()

> +		chip->channel_temp = iio_channel_get(&client->dev, "temp");

devm_iio_channel_get()

> +		if (IS_ERR(chip->channel_temp))
> +			return dev_err_probe(&client->dev, PTR_ERR(chip->channel_temp),
> +					     "failed to get temp\n");
> +	};

Also this must be acquired before registering the power-supply device.

-- Sebastian

> +
>  	return 0;
>  }
>  
> +static void max17040_remove(struct i2c_client *client)
> +{
> +	struct max17040_chip *chip = i2c_get_clientdata(client);
> +
> +	if (chip->channel_temp)
> +		iio_channel_release(chip->channel_temp);
> +}
> +
>  #ifdef CONFIG_PM_SLEEP
>  
>  static int max17040_suspend(struct device *dev)
> @@ -642,6 +665,7 @@ static struct i2c_driver max17040_i2c_driver = {
>  		.pm	= MAX17040_PM_OPS,
>  	},
>  	.probe_new	= max17040_probe,
> +	.remove		= max17040_remove,
>  	.id_table	= max17040_id,
>  };
>  module_i2c_driver(max17040_i2c_driver);
> -- 
> 2.37.2
> 

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

Re: [PATCH v1 3/4] power: max17040: add passing props from supplier

[Sebastian Reichel]

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

Hi,

On Wed, Mar 08, 2023 at 10:44:18AM +0200, Svyatoslav Ryhel wrote:
> Optionally pass status and health from supplier if
> it supports those props. If cell is online assume it
> is present as well.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>

Charger health might be different from battery health, so it's not
safe to inherit. Otherwise LGTM.

-- Sebastian

> ---
>  drivers/power/supply/max17040_battery.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
> index 2778ed5b5c14..6dfce7b1309e 100644
> --- a/drivers/power/supply/max17040_battery.c
> +++ b/drivers/power/supply/max17040_battery.c
> @@ -390,6 +390,7 @@ static int max17040_get_property(struct power_supply *psy,
>  
>  	switch (psp) {
>  	case POWER_SUPPLY_PROP_ONLINE:
> +	case POWER_SUPPLY_PROP_PRESENT:
>  		val->intval = max17040_get_online(chip);
>  		break;
>  	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> @@ -402,6 +403,10 @@ static int max17040_get_property(struct power_supply *psy,
>  		val->intval = chip->low_soc_alert;
>  		break;
>  
> +	case POWER_SUPPLY_PROP_STATUS:
> +	case POWER_SUPPLY_PROP_HEALTH:
> +		power_supply_get_property_from_supplier(psy, psp, val);
> +		break;
>  	case POWER_SUPPLY_PROP_TECHNOLOGY:
>  		val->intval = chip->batt_info->technology;
>  		break;
> @@ -438,10 +443,13 @@ static const struct regmap_config max17040_regmap = {
>  
>  static enum power_supply_property max17040_battery_props[] = {
>  	POWER_SUPPLY_PROP_ONLINE,
> +	POWER_SUPPLY_PROP_PRESENT,
>  	POWER_SUPPLY_PROP_VOLTAGE_NOW,
>  	POWER_SUPPLY_PROP_CAPACITY,
>  	POWER_SUPPLY_PROP_CAPACITY_ALERT_MIN,
>  	POWER_SUPPLY_PROP_TECHNOLOGY,
> +	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_HEALTH,
>  	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
>  	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
>  	POWER_SUPPLY_PROP_TEMP_MIN,
> -- 
> 2.37.2
> 

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