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
Add simple cell, status, health and temperature properties.
Signed-off-by: Svyatoslav Ryhel <[email protected]>
---
.../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
Simple battery cell allows to pass some constant data
about battery controlled by this fuel gauge.
Signed-off-by: Svyatoslav Ryhel <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On 08/03/2023 09:44, Svyatoslav Ryhel wrote:
> Add simple cell, status, health and temperature properties.
>
> Signed-off-by: Svyatoslav Ryhel <[email protected]>
> ---
> .../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
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 <[email protected]>
> ---
> 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
ср, 8 бер. 2023 р. о 11:04 Krzysztof Kozlowski
<[email protected]> пише:
>
> On 08/03/2023 09:44, Svyatoslav Ryhel wrote:
> > Add simple cell, status, health and temperature properties.
> >
> > Signed-off-by: Svyatoslav Ryhel <[email protected]>
> > ---
> > .../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.
ср, 8 бер. 2023 р. о 11:08 Krzysztof Kozlowski
<[email protected]> пише:
>
> 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 <[email protected]>
> > ---
> > 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
>
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
On 08/03/2023 10:23, Svyatoslav Ryhel wrote:
> ср, 8 бер. 2023 р. о 11:08 Krzysztof Kozlowski
> <[email protected]> пише:
>>
>> 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 <[email protected]>
>>> ---
>>> 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
ср, 8 бер. 2023 р. о 12:44 Krzysztof Kozlowski
<[email protected]> пише:
>
> 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
>
On 08/03/2023 11:51, Svyatoslav Ryhel wrote:
> ср, 8 бер. 2023 р. о 12:44 Krzysztof Kozlowski
> <[email protected]> пише:
>>
>> 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
ср, 8 бер. 2023 р. о 12:53 Krzysztof Kozlowski
<[email protected]> пише:
>
> On 08/03/2023 11:51, Svyatoslav Ryhel wrote:
> > ср, 8 бер. 2023 р. о 12:44 Krzysztof Kozlowski
> > <[email protected]> пише:
> >>
> >> 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
>
On 08/03/2023 12:06, Svyatoslav Ryhel wrote:
> ср, 8 бер. 2023 р. о 12:53 Krzysztof Kozlowski
> <[email protected]> пише:
>>
>> On 08/03/2023 11:51, Svyatoslav Ryhel wrote:
>>> ср, 8 бер. 2023 р. о 12:44 Krzysztof Kozlowski
>>> <[email protected]> пише:
>>>>
>>>> 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
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/[email protected]/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 <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
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/[email protected]/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 <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
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/[email protected]/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 <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
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 <[email protected]>
> ---
> .../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
>
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 <[email protected]>
> ---
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
>
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 <[email protected]>
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
>
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 <[email protected]>
> ---
> 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
>