This series add IRQ handler for low level SOC alert, define a devicetree
binding attribute to configure the alert level threshold and check for changes
in SOC for send uevents.
Max17040 have a pin for alert host about low level state of charge and this
alert can be configured in a threshold from 1% up to 32% of SOC.
Tested on Raspberry Pi Zero W, with a SparkFun Lipo Fuel Gauge module based on
MAXIM MAX17043.
Matheus Castello (4):
power: supply: max17040: Add IRQ handler for low SOC alert
power: supply: max17040: Config alert SOC low level threshold from FDT
dt-bindings: power: supply: Max17040: Add low level SOC alert threshold
power: supply: max17040: Send uevent in SOC changes
.../bindings/power/supply/max17040_battery.txt | 24 ++++++
drivers/power/supply/max17040_battery.c | 95 ++++++++++++++++++++++
2 files changed, 119 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt
--
2.13.3
According datasheet max17040 has a pin for alert host for low SOC.
This pin can be used as external interrupt, so we need to check for
interrupts assigned for device and handle it.
In hadler we are checking and storing fuel gauge registers values
and send an uevent to notificate user space, so user space can decide
save work or turn off since the alert demonstrate that the battery may
no have the power to keep the system turned on for much longer.
Signed-off-by: Matheus Castello <[email protected]>
---
drivers/power/supply/max17040_battery.c | 51 +++++++++++++++++++++++++++++++++
1 file changed, 51 insertions(+)
diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
index 33c40f79d23d..6e54e58814a9 100644
--- a/drivers/power/supply/max17040_battery.c
+++ b/drivers/power/supply/max17040_battery.c
@@ -17,6 +17,7 @@
#include <linux/err.h>
#include <linux/i2c.h>
#include <linux/delay.h>
+#include <linux/interrupt.h>
#include <linux/power_supply.h>
#include <linux/max17040_battery.h>
#include <linux/slab.h>
@@ -179,6 +180,24 @@ static void max17040_work(struct work_struct *work)
MAX17040_DELAY);
}
+static irqreturn_t max17040_thread_handler(int id, void *dev)
+{
+ struct max17040_chip *chip = dev;
+ struct i2c_client *client = chip->client;
+
+ dev_warn(&client->dev, "IRQ: Alert battery low level");
+ /* read registers */
+ max17040_get_vcell(chip->client);
+ max17040_get_soc(chip->client);
+ max17040_get_online(chip->client);
+ max17040_get_status(chip->client);
+
+ /* send uevent */
+ power_supply_changed(chip->battery);
+
+ return IRQ_HANDLED;
+}
+
static enum power_supply_property max17040_battery_props[] = {
POWER_SUPPLY_PROP_STATUS,
POWER_SUPPLY_PROP_ONLINE,
@@ -200,6 +219,8 @@ static int max17040_probe(struct i2c_client *client,
struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
struct power_supply_config psy_cfg = {};
struct max17040_chip *chip;
+ int ret;
+ unsigned int flags;
if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
return -EIO;
@@ -221,6 +242,24 @@ static int max17040_probe(struct i2c_client *client,
return PTR_ERR(chip->battery);
}
+ /* check interrupt */
+ if (client->irq) {
+ dev_info(&client->dev, "IRQ: enabled\n");
+ flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT;
+
+ ret = devm_request_threaded_irq(&client->dev, client->irq,
+ NULL,
+ max17040_thread_handler, flags,
+ chip->battery->desc->name,
+ chip);
+ if (ret) {
+ client->irq = 0;
+ if (ret != -EBUSY)
+ dev_warn(&client->dev,
+ "Failed to get IRQ err %d \n", ret);
+ }
+ }
+
max17040_reset(client);
max17040_get_version(client);
@@ -248,6 +287,12 @@ static int max17040_suspend(struct device *dev)
struct max17040_chip *chip = i2c_get_clientdata(client);
cancel_delayed_work(&chip->work);
+
+ if (chip->client->irq) {
+ disable_irq(chip->client->irq);
+ enable_irq_wake(chip->client->irq);
+ }
+
return 0;
}
@@ -258,6 +303,12 @@ static int max17040_resume(struct device *dev)
queue_delayed_work(system_power_efficient_wq, &chip->work,
MAX17040_DELAY);
+
+ if (chip->client->irq) {
+ disable_irq_wake(chip->client->irq);
+ enable_irq(chip->client->irq);
+ }
+
return 0;
}
--
2.13.3
For configuration of fuel gauge alert for a low level state of charge
interrupt we add a function to config level threshold and a device tree
binding property to set it in flatned device tree node.
Now we can use "maxim,alert-soc-level" property with the values from
1 up to 32 to configure alert interrupt threshold.
Signed-off-by: Matheus Castello <[email protected]>
---
drivers/power/supply/max17040_battery.c | 36 +++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
index 6e54e58814a9..3efa52d32b44 100644
--- a/drivers/power/supply/max17040_battery.c
+++ b/drivers/power/supply/max17040_battery.c
@@ -47,6 +47,8 @@ struct max17040_chip {
int soc;
/* State Of Charge */
int status;
+ /* Alert threshold */
+ int alert_threshold;
};
static int max17040_get_property(struct power_supply *psy,
@@ -123,6 +125,28 @@ static void max17040_get_soc(struct i2c_client *client)
chip->soc = (soc >> 8);
}
+static int max17040_set_soc_threshold(struct i2c_client *client, u8 level)
+{
+ int ret;
+ u16 data;
+
+ /* check if level is between 1% and 32% */
+ if (level > 0 && level < 33) {
+ /* alert threshold use LSb 5 bits from RCOMP */
+ /* two's-complements form: 00000 = 32% and 11111 = 1% */
+ level = 32 - level;
+ data = max17040_read_reg(client, MAX17040_RCOMP);
+ data &= 0xFFE0;
+ data |= level;
+ max17040_write_reg(client, MAX17040_RCOMP, data);
+ ret = 0;
+ } else {
+ ret = -EINVAL;
+ }
+
+ return ret;
+}
+
static void max17040_get_version(struct i2c_client *client)
{
u16 version;
@@ -165,6 +189,16 @@ static void max17040_get_status(struct i2c_client *client)
chip->status = POWER_SUPPLY_STATUS_FULL;
}
+static void max17040_get_of_data(struct max17040_chip *chip)
+{
+ struct device *dev = &chip->client->dev;
+ struct device_node *np = dev->of_node;
+
+ if (of_property_read_s32(np, "maxim,alert-soc-level",
+ &chip->alert_threshold))
+ chip->alert_threshold = 4;
+}
+
static void max17040_work(struct work_struct *work)
{
struct max17040_chip *chip;
@@ -231,6 +265,7 @@ static int max17040_probe(struct i2c_client *client,
chip->client = client;
chip->pdata = client->dev.platform_data;
+ max17040_get_of_data(chip);
i2c_set_clientdata(client, chip);
psy_cfg.drv_data = chip;
@@ -262,6 +297,7 @@ static int max17040_probe(struct i2c_client *client,
max17040_reset(client);
max17040_get_version(client);
+ max17040_set_soc_threshold(client, chip->alert_threshold);
INIT_DEFERRABLE_WORK(&chip->work, max17040_work);
queue_delayed_work(system_power_efficient_wq, &chip->work,
--
2.13.3
Add check for changes in state of charge from delayed work, so
in case of changes we call power_supply_changed to send an uevent.
power_supply_changed send an uevent for alert user space about
changes, is useful for example to user space apps made changes in
UI battery level or made other decisions.
Signed-off-by: Matheus Castello <[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 3efa52d32b44..72915ac9e13b 100644
--- a/drivers/power/supply/max17040_battery.c
+++ b/drivers/power/supply/max17040_battery.c
@@ -202,14 +202,22 @@ static void max17040_get_of_data(struct max17040_chip *chip)
static void max17040_work(struct work_struct *work)
{
struct max17040_chip *chip;
+ u16 last_soc;
chip = container_of(work, struct max17040_chip, work.work);
+ /* store SOC for check change */
+ last_soc = chip->soc;
+
max17040_get_vcell(chip->client);
max17040_get_soc(chip->client);
max17040_get_online(chip->client);
max17040_get_status(chip->client);
+ /* check changes and send uevent */
+ if (last_soc != chip->soc)
+ power_supply_changed(chip->battery);
+
queue_delayed_work(system_power_efficient_wq, &chip->work,
MAX17040_DELAY);
}
--
2.13.3
For configure low level state of charge threshold alert signaled from
max17040 we add "maxim,alert-soc-level" property.
Signed-off-by: Matheus Castello <[email protected]>
---
.../bindings/power/supply/max17040_battery.txt | 24 ++++++++++++++++++++++
1 file changed, 24 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt
diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
new file mode 100644
index 000000000000..e6e4e46c03c4
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
@@ -0,0 +1,24 @@
+max17040_battery
+~~~~~~~~~~~~~~~~
+
+Required properties :
+ - compatible : "maxim,max17040"
+
+Optional threshold properties :
+ If skipped the power up default value will be used
+ - maxim,alert-soc-level : The alert threshold that sets the state of
+ charge level where an interrupt is generated.
+ max17040 can be configured from 1 up to 32.
+
+Remembering that for the interrupt to be handled it must also be described the
+information of the interruption in the node.
+
+Example:
+
+ battery-charger@36 {
+ compatible = "maxim,max17040";
+ reg = <0x36>;
+ maxim,alert-soc-level = <10>;
+ interrupt-parent = <&gpio>;
+ interrupts = <18 IRQ_TYPE_EDGE_FALLING>;
+ };
--
2.13.3
On 23 July 2018 at 06:08, Matheus Castello <[email protected]> wrote:
> According datasheet max17040 has a pin for alert host for low SOC.
> This pin can be used as external interrupt, so we need to check for
> interrupts assigned for device and handle it.
>
> In hadler we are checking and storing fuel gauge registers values
> and send an uevent to notificate user space, so user space can decide
> save work or turn off since the alert demonstrate that the battery may
> no have the power to keep the system turned on for much longer.
>
> Signed-off-by: Matheus Castello <[email protected]>
> ---
> drivers/power/supply/max17040_battery.c | 51 +++++++++++++++++++++++++++++++++
> 1 file changed, 51 insertions(+)
>
> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
> index 33c40f79d23d..6e54e58814a9 100644
> --- a/drivers/power/supply/max17040_battery.c
> +++ b/drivers/power/supply/max17040_battery.c
> @@ -17,6 +17,7 @@
> #include <linux/err.h>
> #include <linux/i2c.h>
> #include <linux/delay.h>
> +#include <linux/interrupt.h>
> #include <linux/power_supply.h>
> #include <linux/max17040_battery.h>
> #include <linux/slab.h>
> @@ -179,6 +180,24 @@ static void max17040_work(struct work_struct *work)
> MAX17040_DELAY);
> }
>
> +static irqreturn_t max17040_thread_handler(int id, void *dev)
> +{
> + struct max17040_chip *chip = dev;
> + struct i2c_client *client = chip->client;
> +
> + dev_warn(&client->dev, "IRQ: Alert battery low level");
> + /* read registers */
> + max17040_get_vcell(chip->client);
You just stored chip->client in client...
> + max17040_get_soc(chip->client);
> + max17040_get_online(chip->client);
> + max17040_get_status(chip->client);
This duplicates max17040_work(). Can you split common part?
> +
> + /* send uevent */
> + power_supply_changed(chip->battery);
> +
> + return IRQ_HANDLED;
> +}
> +
> static enum power_supply_property max17040_battery_props[] = {
> POWER_SUPPLY_PROP_STATUS,
> POWER_SUPPLY_PROP_ONLINE,
> @@ -200,6 +219,8 @@ static int max17040_probe(struct i2c_client *client,
> struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> struct power_supply_config psy_cfg = {};
> struct max17040_chip *chip;
> + int ret;
> + unsigned int flags;
Define them in scope using them.
>
> if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
> return -EIO;
> @@ -221,6 +242,24 @@ static int max17040_probe(struct i2c_client *client,
> return PTR_ERR(chip->battery);
> }
>
> + /* check interrupt */
> + if (client->irq) {
> + dev_info(&client->dev, "IRQ: enabled\n");
> + flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT;
> +
> + ret = devm_request_threaded_irq(&client->dev, client->irq,
> + NULL,
> + max17040_thread_handler, flags,
> + chip->battery->desc->name,
> + chip);
Please indent it to parenthesis.
> + if (ret) {
> + client->irq = 0;
> + if (ret != -EBUSY)
> + dev_warn(&client->dev,
> + "Failed to get IRQ err %d \n", ret);
> + }
> + }
> +
> max17040_reset(client);
> max17040_get_version(client);
>
> @@ -248,6 +287,12 @@ static int max17040_suspend(struct device *dev)
> struct max17040_chip *chip = i2c_get_clientdata(client);
>
> cancel_delayed_work(&chip->work);
> +
> + if (chip->client->irq) {
I think this should use device wakeup properties (e.g.
device_init_wakeup(), device_may_wakeup()) coming from pdata.
It would be nice to CC users of this driver. We have it on some of
boards. Unfortunately get_maintainer will not point it automatically
so:
scripts/get_maintainer.pl -f drivers/mfd/max14577.c
Best regards,
Krzysztof
> + disable_irq(chip->client->irq);
> + enable_irq_wake(chip->client->irq);
> + }
> +
> return 0;
> }
>
> @@ -258,6 +303,12 @@ static int max17040_resume(struct device *dev)
>
> queue_delayed_work(system_power_efficient_wq, &chip->work,
> MAX17040_DELAY);
> +
> + if (chip->client->irq) {
> + disable_irq_wake(chip->client->irq);
> + enable_irq(chip->client->irq);
> + }
> +
> return 0;
> }
>
> --
> 2.13.3
>
On 23 July 2018 at 06:08, Matheus Castello <[email protected]> wrote:
> For configuration of fuel gauge alert for a low level state of charge
> interrupt we add a function to config level threshold and a device tree
> binding property to set it in flatned device tree node.
>
> Now we can use "maxim,alert-soc-level" property with the values from
> 1 up to 32 to configure alert interrupt threshold.
>
> Signed-off-by: Matheus Castello <[email protected]>
> ---
> drivers/power/supply/max17040_battery.c | 36 +++++++++++++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
Hi Matheus,
In series, bindings describing new DT property should go before
introducing them in the driver.
>
> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
> index 6e54e58814a9..3efa52d32b44 100644
> --- a/drivers/power/supply/max17040_battery.c
> +++ b/drivers/power/supply/max17040_battery.c
> @@ -47,6 +47,8 @@ struct max17040_chip {
> int soc;
> /* State Of Charge */
> int status;
> + /* Alert threshold */
Threshold in what units?
> + int alert_threshold;
> };
>
> static int max17040_get_property(struct power_supply *psy,
> @@ -123,6 +125,28 @@ static void max17040_get_soc(struct i2c_client *client)
> chip->soc = (soc >> 8);
> }
>
> +static int max17040_set_soc_threshold(struct i2c_client *client, u8 level)
> +{
> + int ret;
> + u16 data;
> +
> + /* check if level is between 1% and 32% */
> + if (level > 0 && level < 33) {
> + /* alert threshold use LSb 5 bits from RCOMP */
> + /* two's-complements form: 00000 = 32% and 11111 = 1% */
Please use Linux style comments.
> + level = 32 - level;
> + data = max17040_read_reg(client, MAX17040_RCOMP);
> + data &= 0xFFE0;
Please define the mask.
> + data |= level;
> + max17040_write_reg(client, MAX17040_RCOMP, data);
> + ret = 0;
> + } else {
> + ret = -EINVAL;
> + }
> +
> + return ret;
> +}
> +
> static void max17040_get_version(struct i2c_client *client)
> {
> u16 version;
> @@ -165,6 +189,16 @@ static void max17040_get_status(struct i2c_client *client)
> chip->status = POWER_SUPPLY_STATUS_FULL;
> }
>
> +static void max17040_get_of_data(struct max17040_chip *chip)
> +{
> + struct device *dev = &chip->client->dev;
> + struct device_node *np = dev->of_node;
> +
> + if (of_property_read_s32(np, "maxim,alert-soc-level",
> + &chip->alert_threshold))
> + chip->alert_threshold = 4;
1. You read s32 and later cast it to u8. Either some validation of
possible values or of_property_read_u8().
2. You have hard-coded default value - please put it in some define.
There are already defines, e.g.: MAX17040_BATTERY_FULL
3. Also the bindings say something about power up value... not 4.
4, I think that default - missing - value should mean "no interrupt warnings".
> +}
> +
> static void max17040_work(struct work_struct *work)
> {
> struct max17040_chip *chip;
> @@ -231,6 +265,7 @@ static int max17040_probe(struct i2c_client *client,
>
> chip->client = client;
> chip->pdata = client->dev.platform_data;
> + max17040_get_of_data(chip);
>
> i2c_set_clientdata(client, chip);
> psy_cfg.drv_data = chip;
> @@ -262,6 +297,7 @@ static int max17040_probe(struct i2c_client *client,
>
> max17040_reset(client);
> max17040_get_version(client);
> + max17040_set_soc_threshold(client, chip->alert_threshold);
1. Return value ignored.
2. First you enable interrupts for whatever default value of alerts
and then you set the alerts threshold. You might generate false
warning in such case so this should be in reverse order.
Best regards,
Krzysztof
>
> INIT_DEFERRABLE_WORK(&chip->work, max17040_work);
> queue_delayed_work(system_power_efficient_wq, &chip->work,
> --
> 2.13.3
>
On 23 July 2018 at 06:08, Matheus Castello <[email protected]> wrote:
> For configure low level state of charge threshold alert signaled from
> max17040 we add "maxim,alert-soc-level" property.
>
> Signed-off-by: Matheus Castello <[email protected]>
> ---
> .../bindings/power/supply/max17040_battery.txt | 24 ++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt
>
> diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
> new file mode 100644
> index 000000000000..e6e4e46c03c4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
> @@ -0,0 +1,24 @@
> +max17040_battery
> +~~~~~~~~~~~~~~~~
> +
> +Required properties :
> + - compatible : "maxim,max17040"
Why you skipped "maxim,max77836-battery"?
> +
> +Optional threshold properties :
> + If skipped the power up default value will be used
> + - maxim,alert-soc-level : The alert threshold that sets the state of
> + charge level where an interrupt is generated.
> + max17040 can be configured from 1 up to 32.
> +
> +Remembering that for the interrupt to be handled it must also be described the
> +information of the interruption in the node.
Just mention interrupts in optional properties, including the flags.
BTW, the driver hard-codes the flags which is in contrast with DT
here.
Best regards,
Krzysztof
> +
> +Example:
> +
> + battery-charger@36 {
> + compatible = "maxim,max17040";
> + reg = <0x36>;
> + maxim,alert-soc-level = <10>;
> + interrupt-parent = <&gpio>;
> + interrupts = <18 IRQ_TYPE_EDGE_FALLING>;
> + };
> --
> 2.13.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23 July 2018 at 06:08, Matheus Castello <[email protected]> wrote:
> Add check for changes in state of charge from delayed work, so
> in case of changes we call power_supply_changed to send an uevent.
>
> power_supply_changed send an uevent for alert user space about
> changes, is useful for example to user space apps made changes in
> UI battery level or made other decisions.
Hi,
Too many "changes". How about:
Notify core through power_supply_changed() in case of changes in state
of charge. This is useful for user-space to efficiently update current
battery level.
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
> Signed-off-by: Matheus Castello <[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 3efa52d32b44..72915ac9e13b 100644
> --- a/drivers/power/supply/max17040_battery.c
> +++ b/drivers/power/supply/max17040_battery.c
> @@ -202,14 +202,22 @@ static void max17040_get_of_data(struct max17040_chip *chip)
> static void max17040_work(struct work_struct *work)
> {
> struct max17040_chip *chip;
> + u16 last_soc;
>
> chip = container_of(work, struct max17040_chip, work.work);
>
> + /* store SOC for check change */
> + last_soc = chip->soc;
> +
> max17040_get_vcell(chip->client);
> max17040_get_soc(chip->client);
> max17040_get_online(chip->client);
> max17040_get_status(chip->client);
>
> + /* check changes and send uevent */
> + if (last_soc != chip->soc)
> + power_supply_changed(chip->battery);
> +
> queue_delayed_work(system_power_efficient_wq, &chip->work,
> MAX17040_DELAY);
> }
> --
> 2.13.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Matheus,
Did I miss a v2 of this patchset, that solves the issues
found by Krzysztof?
-- Sebastian
On Mon, Jul 23, 2018 at 12:08:12AM -0400, Matheus Castello wrote:
> This series add IRQ handler for low level SOC alert, define a devicetree
> binding attribute to configure the alert level threshold and check for changes
> in SOC for send uevents.
>
> Max17040 have a pin for alert host about low level state of charge and this
> alert can be configured in a threshold from 1% up to 32% of SOC.
>
> Tested on Raspberry Pi Zero W, with a SparkFun Lipo Fuel Gauge module based on
> MAXIM MAX17043.
>
> Matheus Castello (4):
> power: supply: max17040: Add IRQ handler for low SOC alert
> power: supply: max17040: Config alert SOC low level threshold from FDT
> dt-bindings: power: supply: Max17040: Add low level SOC alert threshold
> power: supply: max17040: Send uevent in SOC changes
>
> .../bindings/power/supply/max17040_battery.txt | 24 ++++++
> drivers/power/supply/max17040_battery.c | 95 ++++++++++++++++++++++
> 2 files changed, 119 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt
>
> --
> 2.13.3
>
On Sun, 16 Sep 2018 at 22:03, Sebastian Reichel
<[email protected]> wrote:
>
> Hi Matheus,
>
> Did I miss a v2 of this patchset, that solves the issues
> found by Krzysztof?
I did not see v2. This patchset brings nice feature so it would be a
pity if it were discontinued.
Best regards,
Krzysztof
Hi Krzysztof and Sebastian,
please forgive me for the delay in working with this patch.
I've been having some personal issues these months, but leaving the
excuses I still intend to send a v2 for this.
Thanks Krzysztof for review and tips I'll work on it.
Best Regards,
Matheus Castello
On 09/17/2018 08:32 AM, Krzysztof Kozlowski wrote:
> On Sun, 16 Sep 2018 at 22:03, Sebastian Reichel
> <[email protected]> wrote:
>>
>> Hi Matheus,
>>
>> Did I miss a v2 of this patchset, that solves the issues
>> found by Krzysztof?
>
> I did not see v2. This patchset brings nice feature so it would be a
> pity if it were discontinued.
>
> Best regards,
> Krzysztof
>
Notify core through power_supply_changed() in case of changes in state
of charge. This is useful for user-space to efficiently update current
battery level.
Signed-off-by: Matheus Castello <[email protected]>
---
drivers/power/supply/max17040_battery.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
index f036f272d52f..db901ebf495d 100644
--- a/drivers/power/supply/max17040_battery.c
+++ b/drivers/power/supply/max17040_battery.c
@@ -208,10 +208,17 @@ static void max17040_check_changes(struct i2c_client *client)
static void max17040_work(struct work_struct *work)
{
struct max17040_chip *chip;
+ int last_soc;
chip = container_of(work, struct max17040_chip, work.work);
+ /* store SOC for check change */
+ last_soc = chip->soc;
max17040_check_changes(chip->client);
+ /* check changes and send uevent */
+ if (last_soc != chip->soc)
+ power_supply_changed(chip->battery);
+
queue_delayed_work(system_power_efficient_wq, &chip->work,
MAX17040_DELAY);
}
--
2.17.0
This series add IRQ handler for low level SOC alert, define a devicetree
binding attribute to configure the alert level threshold and check for
changes in SOC for send uevents.
Max17040 have a pin for alert host about low level state of charge and
this alert can be configured in a threshold from 1% up to 32% of SOC.
Tested on Toradex Colibri iMX7D, with a SparkFun Lipo Fuel Gauge module
based on MAXIM MAX17043.
Thanks Krzysztof Kozlowski for your time reviewing it, and forgive me for
the delay in working on it, now I'm back to the patchs. Let me know what
you think about the fixes and I'm open to maintainers suggestions.
Changes since v1:
(Suggested by Krzysztof Kozlowski)
- Put common code from max17040_work and max17040_thread_handler in a function
- Code style fixes
- Define mask and low level threshold alert default
- Check return value from max17040_set_soc_threshold
- Set low level state of charge threshold before IRQ
- CC maintainers from drivers/mfd/max14577
- Use flags from FDT client->flags instead hard coded it
- Mention interrupts on DT Documentation
- Fix "maxim,max77836-battery" missed from DT Documentation
- Fix commit description
Matheus Castello (4):
power: supply: max17040: Add IRQ handler for low SOC alert
dt-bindings: power: supply: Max17040: Add low level SOC alert
threshold
power: supply: max17040: Config alert SOC low level threshold from FDT
power: supply: max17040: Send uevent in SOC changes
.../power/supply/max17040_battery.txt | 24 ++++
drivers/power/supply/max17040_battery.c | 118 +++++++++++++++++-
2 files changed, 138 insertions(+), 4 deletions(-)
create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt
--
2.17.0
For configuration of fuel gauge alert for a low level state of charge
interrupt we add a function to config level threshold and a device tree
binding property to set it in flatned device tree node.
Now we can use "maxim,alert-soc-level" property with the values from
1 up to 32 to configure alert interrupt threshold.
Signed-off-by: Matheus Castello <[email protected]>
---
drivers/power/supply/max17040_battery.c | 56 ++++++++++++++++++++++---
1 file changed, 50 insertions(+), 6 deletions(-)
diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
index 8d2f8ed3f44c..f036f272d52f 100644
--- a/drivers/power/supply/max17040_battery.c
+++ b/drivers/power/supply/max17040_battery.c
@@ -29,6 +29,9 @@
#define MAX17040_DELAY 1000
#define MAX17040_BATTERY_FULL 95
+#define MAX17040_ATHD_MASK 0xFFE0
+#define MAX17040_ATHD_DEFAULT_POWER_UP 4
+
struct max17040_chip {
struct i2c_client *client;
struct delayed_work work;
@@ -43,6 +46,8 @@ struct max17040_chip {
int soc;
/* State Of Charge */
int status;
+ /* Alert threshold from 32% to 1% of the State of Charge */
+ u32 alert_threshold;
};
static int max17040_get_property(struct power_supply *psy,
@@ -119,6 +124,27 @@ static void max17040_get_soc(struct i2c_client *client)
chip->soc = (soc >> 8);
}
+static int max17040_set_soc_threshold(struct i2c_client *client, u32 level)
+{
+ int ret;
+ u16 data;
+
+ /* check if level is between 1% and 32% */
+ if (level > 0 && level < 33) {
+ /* alert threshold use LSb 5 bits from RCOMP */
+ level = 32 - level;
+ data = max17040_read_reg(client, MAX17040_RCOMP);
+ data &= MAX17040_ATHD_MASK;
+ data |= level;
+ max17040_write_reg(client, MAX17040_RCOMP, data);
+ ret = 0;
+ } else {
+ ret = -EINVAL;
+ }
+
+ return ret;
+}
+
static void max17040_get_version(struct i2c_client *client)
{
u16 version;
@@ -161,6 +187,16 @@ static void max17040_get_status(struct i2c_client *client)
chip->status = POWER_SUPPLY_STATUS_FULL;
}
+static void max17040_get_of_data(struct max17040_chip *chip)
+{
+ struct device *dev = &chip->client->dev;
+ struct device_node *np = dev->of_node;
+
+ if (of_property_read_u32(np, "maxim,alert-soc-level",
+ &chip->alert_threshold))
+ chip->alert_threshold = MAX17040_ATHD_DEFAULT_POWER_UP;
+}
+
static void max17040_check_changes(struct i2c_client *client)
{
max17040_get_vcell(client);
@@ -226,6 +262,7 @@ static int max17040_probe(struct i2c_client *client,
chip->client = client;
chip->pdata = client->dev.platform_data;
+ max17040_get_of_data(chip);
i2c_set_clientdata(client, chip);
psy_cfg.drv_data = chip;
@@ -237,16 +274,26 @@ static int max17040_probe(struct i2c_client *client,
return PTR_ERR(chip->battery);
}
+ max17040_reset(client);
+ max17040_get_version(client);
+
/* check interrupt */
if (client->irq) {
int ret;
- unsigned int flags;
+
+ ret = max17040_set_soc_threshold(client, chip->alert_threshold);
+ if (ret) {
+ dev_err(&client->dev,
+ "Failed to set SOC alert threshold: err %d\n",
+ ret);
+ return ret;
+ }
dev_info(&client->dev, "IRQ: enabled\n");
- flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT;
ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
- max17040_thread_handler, flags,
+ max17040_thread_handler,
+ (client->flags | IRQF_ONESHOT),
chip->battery->desc->name,
chip);
@@ -258,9 +305,6 @@ static int max17040_probe(struct i2c_client *client,
}
}
- max17040_reset(client);
- max17040_get_version(client);
-
INIT_DEFERRABLE_WORK(&chip->work, max17040_work);
queue_delayed_work(system_power_efficient_wq, &chip->work,
MAX17040_DELAY);
--
2.17.0
For configure low level state of charge threshold alert signaled from
max17040 we add "maxim,alert-soc-level" property.
Signed-off-by: Matheus Castello <[email protected]>
---
.../power/supply/max17040_battery.txt | 24 +++++++++++++++++++
1 file changed, 24 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt
diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
new file mode 100644
index 000000000000..9b2cc67d556f
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
@@ -0,0 +1,24 @@
+max17040_battery
+~~~~~~~~~~~~~~~~
+
+Required properties :
+ - compatible : "maxim,max17040" or "maxim,max77836-battery"
+
+Optional properties :
+- maxim,alert-soc-level : The alert threshold that sets the state of
+ charge level where an interrupt is generated.
+ Can be configured from 1 up to 32. If skipped
+ the power up default value of 4 will be used.
+- interrupt-parent : The GPIO bank from the interrupt line.
+- interrupts : Interrupt line see Documentation/devicetree/
+ bindings/interrupt-controller/interrupts.txt
+
+Example:
+
+ battery-charger@36 {
+ compatible = "maxim,max17040";
+ reg = <0x36>;
+ maxim,alert-soc-level = <10>;
+ interrupt-parent = <&gpio7>;
+ interrupts = <2 IRQ_TYPE_EDGE_FALLING>;
+ };
--
2.17.0
According datasheet max17040 has a pin for alert host for low SOC.
This pin can be used as external interrupt, so we need to check for
interrupts assigned for device and handle it.
In hadler we are checking and storing fuel gauge registers values
and send an uevent to notificate user space, so user space can decide
save work or turn off since the alert demonstrate that the battery may
no have the power to keep the system turned on for much longer.
Signed-off-by: Matheus Castello <[email protected]>
---
drivers/power/supply/max17040_battery.c | 69 +++++++++++++++++++++++--
1 file changed, 64 insertions(+), 5 deletions(-)
diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
index 91cafc7bed30..8d2f8ed3f44c 100644
--- a/drivers/power/supply/max17040_battery.c
+++ b/drivers/power/supply/max17040_battery.c
@@ -13,6 +13,7 @@
#include <linux/err.h>
#include <linux/i2c.h>
#include <linux/delay.h>
+#include <linux/interrupt.h>
#include <linux/power_supply.h>
#include <linux/max17040_battery.h>
#include <linux/slab.h>
@@ -160,21 +161,40 @@ static void max17040_get_status(struct i2c_client *client)
chip->status = POWER_SUPPLY_STATUS_FULL;
}
+static void max17040_check_changes(struct i2c_client *client)
+{
+ max17040_get_vcell(client);
+ max17040_get_soc(client);
+ max17040_get_online(client);
+ max17040_get_status(client);
+}
+
static void max17040_work(struct work_struct *work)
{
struct max17040_chip *chip;
chip = container_of(work, struct max17040_chip, work.work);
-
- max17040_get_vcell(chip->client);
- max17040_get_soc(chip->client);
- max17040_get_online(chip->client);
- max17040_get_status(chip->client);
+ max17040_check_changes(chip->client);
queue_delayed_work(system_power_efficient_wq, &chip->work,
MAX17040_DELAY);
}
+static irqreturn_t max17040_thread_handler(int id, void *dev)
+{
+ struct max17040_chip *chip = dev;
+ struct i2c_client *client = chip->client;
+
+ dev_warn(&client->dev, "IRQ: Alert battery low level");
+ /* read registers */
+ max17040_check_changes(chip->client);
+
+ /* send uevent */
+ power_supply_changed(chip->battery);
+
+ return IRQ_HANDLED;
+}
+
static enum power_supply_property max17040_battery_props[] = {
POWER_SUPPLY_PROP_STATUS,
POWER_SUPPLY_PROP_ONLINE,
@@ -217,6 +237,27 @@ static int max17040_probe(struct i2c_client *client,
return PTR_ERR(chip->battery);
}
+ /* check interrupt */
+ if (client->irq) {
+ int ret;
+ unsigned int flags;
+
+ dev_info(&client->dev, "IRQ: enabled\n");
+ flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT;
+
+ ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
+ max17040_thread_handler, flags,
+ chip->battery->desc->name,
+ chip);
+
+ if (ret) {
+ client->irq = 0;
+ if (ret != -EBUSY)
+ dev_warn(&client->dev,
+ "Failed to get IRQ err %d\n", ret);
+ }
+ }
+
max17040_reset(client);
max17040_get_version(client);
@@ -224,6 +265,8 @@ static int max17040_probe(struct i2c_client *client,
queue_delayed_work(system_power_efficient_wq, &chip->work,
MAX17040_DELAY);
+ device_init_wakeup(&client->dev, 1);
+
return 0;
}
@@ -244,6 +287,14 @@ static int max17040_suspend(struct device *dev)
struct max17040_chip *chip = i2c_get_clientdata(client);
cancel_delayed_work(&chip->work);
+
+ if (client->irq) {
+ if (device_may_wakeup(dev))
+ enable_irq_wake(client->irq);
+ else
+ disable_irq_wake(client->irq);
+ }
+
return 0;
}
@@ -254,6 +305,14 @@ static int max17040_resume(struct device *dev)
queue_delayed_work(system_power_efficient_wq, &chip->work,
MAX17040_DELAY);
+
+ if (client->irq) {
+ if (device_may_wakeup(dev))
+ disable_irq_wake(client->irq);
+ else
+ enable_irq_wake(client->irq);
+ }
+
return 0;
}
--
2.17.0
On Mon, 15 Apr 2019 at 03:49, Matheus Castello <[email protected]> wrote:
>
> According datasheet max17040 has a pin for alert host for low SOC.
> This pin can be used as external interrupt, so we need to check for
> interrupts assigned for device and handle it.
>
> In hadler we are checking and storing fuel gauge registers values
> and send an uevent to notificate user space, so user space can decide
> save work or turn off since the alert demonstrate that the battery may
> no have the power to keep the system turned on for much longer.
>
> Signed-off-by: Matheus Castello <[email protected]>
> ---
> drivers/power/supply/max17040_battery.c | 69 +++++++++++++++++++++++--
> 1 file changed, 64 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
> index 91cafc7bed30..8d2f8ed3f44c 100644
> --- a/drivers/power/supply/max17040_battery.c
> +++ b/drivers/power/supply/max17040_battery.c
> @@ -13,6 +13,7 @@
> #include <linux/err.h>
> #include <linux/i2c.h>
> #include <linux/delay.h>
> +#include <linux/interrupt.h>
> #include <linux/power_supply.h>
> #include <linux/max17040_battery.h>
> #include <linux/slab.h>
> @@ -160,21 +161,40 @@ static void max17040_get_status(struct i2c_client *client)
> chip->status = POWER_SUPPLY_STATUS_FULL;
> }
>
> +static void max17040_check_changes(struct i2c_client *client)
> +{
> + max17040_get_vcell(client);
> + max17040_get_soc(client);
> + max17040_get_online(client);
> + max17040_get_status(client);
> +}
> +
> static void max17040_work(struct work_struct *work)
> {
> struct max17040_chip *chip;
>
> chip = container_of(work, struct max17040_chip, work.work);
> -
> - max17040_get_vcell(chip->client);
> - max17040_get_soc(chip->client);
> - max17040_get_online(chip->client);
> - max17040_get_status(chip->client);
> + max17040_check_changes(chip->client);
>
> queue_delayed_work(system_power_efficient_wq, &chip->work,
> MAX17040_DELAY);
> }
>
> +static irqreturn_t max17040_thread_handler(int id, void *dev)
> +{
> + struct max17040_chip *chip = dev;
> + struct i2c_client *client = chip->client;
> +
> + dev_warn(&client->dev, "IRQ: Alert battery low level");
> + /* read registers */
> + max17040_check_changes(chip->client);
> +
> + /* send uevent */
> + power_supply_changed(chip->battery);
> +
> + return IRQ_HANDLED;
> +}
> +
> static enum power_supply_property max17040_battery_props[] = {
> POWER_SUPPLY_PROP_STATUS,
> POWER_SUPPLY_PROP_ONLINE,
> @@ -217,6 +237,27 @@ static int max17040_probe(struct i2c_client *client,
> return PTR_ERR(chip->battery);
> }
>
> + /* check interrupt */
> + if (client->irq) {
> + int ret;
> + unsigned int flags;
> +
> + dev_info(&client->dev, "IRQ: enabled\n");
> + flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT;
> +
> + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> + max17040_thread_handler, flags,
> + chip->battery->desc->name,
> + chip);
> +
> + if (ret) {
> + client->irq = 0;
> + if (ret != -EBUSY)
Why not on EBUSY?
> + dev_warn(&client->dev,
> + "Failed to get IRQ err %d\n", ret);
> + }
> + }
> +
> max17040_reset(client);
> max17040_get_version(client);
>
> @@ -224,6 +265,8 @@ static int max17040_probe(struct i2c_client *client,
> queue_delayed_work(system_power_efficient_wq, &chip->work,
> MAX17040_DELAY);
>
> + device_init_wakeup(&client->dev, 1);
Either you parse DT for wakeup-source property and use it... or you
unconditionally enable wakeup. In the second case - there is no point
to check later for device_may_wakeup(). Instead check the return value
of device_init_wakeup().
> +
> return 0;
> }
>
> @@ -244,6 +287,14 @@ static int max17040_suspend(struct device *dev)
> struct max17040_chip *chip = i2c_get_clientdata(client);
>
> cancel_delayed_work(&chip->work);
> +
> + if (client->irq) {
> + if (device_may_wakeup(dev))
> + enable_irq_wake(client->irq);
> + else
> + disable_irq_wake(client->irq);
Did you try the wakeup from suspend? You do not have a disable_irq()
here which usually was needed for interrupts to work properly on
suspend. Maybe this was fixed?
Best regards,
Krzysztof
On Mon, 15 Apr 2019 at 03:50, Matheus Castello <[email protected]> wrote:
>
> For configure low level state of charge threshold alert signaled from
> max17040 we add "maxim,alert-soc-level" property.
>
> Signed-off-by: Matheus Castello <[email protected]>
> ---
> .../power/supply/max17040_battery.txt | 24 +++++++++++++++++++
> 1 file changed, 24 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt
>
> diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
> new file mode 100644
> index 000000000000..9b2cc67d556f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
> @@ -0,0 +1,24 @@
> +max17040_battery
> +~~~~~~~~~~~~~~~~
> +
> +Required properties :
> + - compatible : "maxim,max17040" or "maxim,max77836-battery"
> +
> +Optional properties :
> +- maxim,alert-soc-level : The alert threshold that sets the state of
> + charge level where an interrupt is generated.
> + Can be configured from 1 up to 32. If skipped
> + the power up default value of 4 will be used.
> +- interrupt-parent : The GPIO bank from the interrupt line.
It does not have to be GPIO so:
"phandle of the parent interrupt controller"
> +- interrupts : Interrupt line see Documentation/devicetree/
> + bindings/interrupt-controller/interrupts.txt
> +
> +Example:
> +
> + battery-charger@36 {
It is a fuel gauge, not battery charger.
Best regards,
Krzysztof
> + compatible = "maxim,max17040";
> + reg = <0x36>;
> + maxim,alert-soc-level = <10>;
> + interrupt-parent = <&gpio7>;
> + interrupts = <2 IRQ_TYPE_EDGE_FALLING>;
> + };
> --
> 2.17.0
>
On Mon, 15 Apr 2019 at 03:51, Matheus Castello <[email protected]> wrote:
>
> For configuration of fuel gauge alert for a low level state of charge
> interrupt we add a function to config level threshold and a device tree
> binding property to set it in flatned device tree node.
>
> Now we can use "maxim,alert-soc-level" property with the values from
> 1 up to 32 to configure alert interrupt threshold.
>
> Signed-off-by: Matheus Castello <[email protected]>
> ---
> drivers/power/supply/max17040_battery.c | 56 ++++++++++++++++++++++---
> 1 file changed, 50 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
> index 8d2f8ed3f44c..f036f272d52f 100644
> --- a/drivers/power/supply/max17040_battery.c
> +++ b/drivers/power/supply/max17040_battery.c
> @@ -29,6 +29,9 @@
> #define MAX17040_DELAY 1000
> #define MAX17040_BATTERY_FULL 95
>
> +#define MAX17040_ATHD_MASK 0xFFE0
> +#define MAX17040_ATHD_DEFAULT_POWER_UP 4
> +
> struct max17040_chip {
> struct i2c_client *client;
> struct delayed_work work;
> @@ -43,6 +46,8 @@ struct max17040_chip {
> int soc;
> /* State Of Charge */
> int status;
> + /* Alert threshold from 32% to 1% of the State of Charge */
> + u32 alert_threshold;
> };
>
> static int max17040_get_property(struct power_supply *psy,
> @@ -119,6 +124,27 @@ static void max17040_get_soc(struct i2c_client *client)
> chip->soc = (soc >> 8);
> }
>
> +static int max17040_set_soc_threshold(struct i2c_client *client, u32 level)
> +{
> + int ret;
> + u16 data;
> +
> + /* check if level is between 1% and 32% */
> + if (level > 0 && level < 33) {
> + /* alert threshold use LSb 5 bits from RCOMP */
> + level = 32 - level;
> + data = max17040_read_reg(client, MAX17040_RCOMP);
> + data &= MAX17040_ATHD_MASK;
> + data |= level;
> + max17040_write_reg(client, MAX17040_RCOMP, data);
> + ret = 0;
> + } else {
> + ret = -EINVAL;
> + }
> +
> + return ret;
> +}
> +
> static void max17040_get_version(struct i2c_client *client)
> {
> u16 version;
> @@ -161,6 +187,16 @@ static void max17040_get_status(struct i2c_client *client)
> chip->status = POWER_SUPPLY_STATUS_FULL;
> }
>
> +static void max17040_get_of_data(struct max17040_chip *chip)
> +{
> + struct device *dev = &chip->client->dev;
> + struct device_node *np = dev->of_node;
> +
> + if (of_property_read_u32(np, "maxim,alert-soc-level",
> + &chip->alert_threshold))
> + chip->alert_threshold = MAX17040_ATHD_DEFAULT_POWER_UP;
> +}
> +
> static void max17040_check_changes(struct i2c_client *client)
> {
> max17040_get_vcell(client);
> @@ -226,6 +262,7 @@ static int max17040_probe(struct i2c_client *client,
>
> chip->client = client;
> chip->pdata = client->dev.platform_data;
> + max17040_get_of_data(chip);
>
> i2c_set_clientdata(client, chip);
> psy_cfg.drv_data = chip;
> @@ -237,16 +274,26 @@ static int max17040_probe(struct i2c_client *client,
> return PTR_ERR(chip->battery);
> }
>
> + max17040_reset(client);
> + max17040_get_version(client);
> +
> /* check interrupt */
> if (client->irq) {
> int ret;
> - unsigned int flags;
> +
> + ret = max17040_set_soc_threshold(client, chip->alert_threshold);
> + if (ret) {
> + dev_err(&client->dev,
> + "Failed to set SOC alert threshold: err %d\n",
> + ret);
> + return ret;
> + }
>
> dev_info(&client->dev, "IRQ: enabled\n");
> - flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT;
>
> ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> - max17040_thread_handler, flags,
> + max17040_thread_handler,
> + (client->flags | IRQF_ONESHOT),
> chip->battery->desc->name,
> chip);
Now I noticed that you are not clearing the ALRT status bit. Therefore
alert interrupt will be generated only once. Even if SoC goes up
(charged) and then down to alert level it will not be generated second
time. At least documentation for max77836 is saying that clearing this
bit is required.
Best regards,
Krzysztof
On Mon, 15 Apr 2019 at 03:48, Matheus Castello <[email protected]> wrote:
>
> Notify core through power_supply_changed() in case of changes in state
> of charge. This is useful for user-space to efficiently update current
> battery level.
>
> Signed-off-by: Matheus Castello <[email protected]>
> ---
> drivers/power/supply/max17040_battery.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
> index f036f272d52f..db901ebf495d 100644
> --- a/drivers/power/supply/max17040_battery.c
> +++ b/drivers/power/supply/max17040_battery.c
> @@ -208,10 +208,17 @@ static void max17040_check_changes(struct i2c_client *client)
> static void max17040_work(struct work_struct *work)
> {
> struct max17040_chip *chip;
> + int last_soc;
>
> chip = container_of(work, struct max17040_chip, work.work);
> + /* store SOC for check change */
> + last_soc = chip->soc;
> max17040_check_changes(chip->client);
>
> + /* check changes and send uevent */
> + if (last_soc != chip->soc)
chip->soc could be negative ERRNO so in such case I think user-space
should not be notified.
> + power_supply_changed(chip->battery);
> +
You should also notify on online and status change (e.g. started
charging). User-space also wants to know that, e.g. to show the
charging icon or battery health status.
Best regards,
Krzysztof
Em 4/15/19 4:10 AM, Krzysztof Kozlowski escreveu:
> On Mon, 15 Apr 2019 at 03:49, Matheus Castello <[email protected]> wrote:
>>
>> According datasheet max17040 has a pin for alert host for low SOC.
>> This pin can be used as external interrupt, so we need to check for
>> interrupts assigned for device and handle it.
>>
>> In hadler we are checking and storing fuel gauge registers values
>> and send an uevent to notificate user space, so user space can decide
>> save work or turn off since the alert demonstrate that the battery may
>> no have the power to keep the system turned on for much longer.
>>
>> Signed-off-by: Matheus Castello <[email protected]>
>> ---
>> drivers/power/supply/max17040_battery.c | 69 +++++++++++++++++++++++--
>> 1 file changed, 64 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
>> index 91cafc7bed30..8d2f8ed3f44c 100644
>> --- a/drivers/power/supply/max17040_battery.c
>> +++ b/drivers/power/supply/max17040_battery.c
>> @@ -13,6 +13,7 @@
>> #include <linux/err.h>
>> #include <linux/i2c.h>
>> #include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> #include <linux/power_supply.h>
>> #include <linux/max17040_battery.h>
>> #include <linux/slab.h>
>> @@ -160,21 +161,40 @@ static void max17040_get_status(struct i2c_client *client)
>> chip->status = POWER_SUPPLY_STATUS_FULL;
>> }
>>
>> +static void max17040_check_changes(struct i2c_client *client)
>> +{
>> + max17040_get_vcell(client);
>> + max17040_get_soc(client);
>> + max17040_get_online(client);
>> + max17040_get_status(client);
>> +}
>> +
>> static void max17040_work(struct work_struct *work)
>> {
>> struct max17040_chip *chip;
>>
>> chip = container_of(work, struct max17040_chip, work.work);
>> -
>> - max17040_get_vcell(chip->client);
>> - max17040_get_soc(chip->client);
>> - max17040_get_online(chip->client);
>> - max17040_get_status(chip->client);
>> + max17040_check_changes(chip->client);
>>
>> queue_delayed_work(system_power_efficient_wq, &chip->work,
>> MAX17040_DELAY);
>> }
>>
>> +static irqreturn_t max17040_thread_handler(int id, void *dev)
>> +{
>> + struct max17040_chip *chip = dev;
>> + struct i2c_client *client = chip->client;
>> +
>> + dev_warn(&client->dev, "IRQ: Alert battery low level");
>> + /* read registers */
>> + max17040_check_changes(chip->client);
>> +
>> + /* send uevent */
>> + power_supply_changed(chip->battery);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> static enum power_supply_property max17040_battery_props[] = {
>> POWER_SUPPLY_PROP_STATUS,
>> POWER_SUPPLY_PROP_ONLINE,
>> @@ -217,6 +237,27 @@ static int max17040_probe(struct i2c_client *client,
>> return PTR_ERR(chip->battery);
>> }
>>
>> + /* check interrupt */
>> + if (client->irq) {
>> + int ret;
>> + unsigned int flags;
>> +
>> + dev_info(&client->dev, "IRQ: enabled\n");
>> + flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT;
>> +
>> + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
>> + max17040_thread_handler, flags,
>> + chip->battery->desc->name,
>> + chip);
>> +
>> + if (ret) {
>> + client->irq = 0;
>> + if (ret != -EBUSY)
>
> Why not on EBUSY?
>
>> + dev_warn(&client->dev,
>> + "Failed to get IRQ err %d\n", ret);
>> + }
>> + }
>> +
>> max17040_reset(client);
>> max17040_get_version(client);
>>
>> @@ -224,6 +265,8 @@ static int max17040_probe(struct i2c_client *client,
>> queue_delayed_work(system_power_efficient_wq, &chip->work,
>> MAX17040_DELAY);
>>
>> + device_init_wakeup(&client->dev, 1);
>
> Either you parse DT for wakeup-source property and use it... or you
> unconditionally enable wakeup. In the second case - there is no point
> to check later for device_may_wakeup(). Instead check the return value
> of device_init_wakeup().
>
>> +
>> return 0;
>> }
>>
>> @@ -244,6 +287,14 @@ static int max17040_suspend(struct device *dev)
>> struct max17040_chip *chip = i2c_get_clientdata(client);
>>
>> cancel_delayed_work(&chip->work);
>> +
>> + if (client->irq) {
>> + if (device_may_wakeup(dev))
>> + enable_irq_wake(client->irq);
>> + else
>> + disable_irq_wake(client->irq);
>
> Did you try the wakeup from suspend? You do not have a disable_irq()
> here which usually was needed for interrupts to work properly on
> suspend. Maybe this was fixed?
>
> Best regards,
> Krzysztof
>
Hi Krzysztof,
I test it using mem state and suspend, resuming seems to have worked
correctly ...
Thanks for the review, I'm working in your suggestions and I expect to
send v3 this weekend.
Best Regards,
Matheus Castello
On Sun, Apr 14, 2019 at 10:26:33PM -0300, Matheus Castello wrote:
> For configure low level state of charge threshold alert signaled from
> max17040 we add "maxim,alert-soc-level" property.
>
> Signed-off-by: Matheus Castello <[email protected]>
> ---
> .../power/supply/max17040_battery.txt | 24 +++++++++++++++++++
> 1 file changed, 24 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt
>
> diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
> new file mode 100644
> index 000000000000..9b2cc67d556f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
> @@ -0,0 +1,24 @@
> +max17040_battery
> +~~~~~~~~~~~~~~~~
> +
> +Required properties :
> + - compatible : "maxim,max17040" or "maxim,max77836-battery"
This is really a charger, not a battery.
> +
> +Optional properties :
> +- maxim,alert-soc-level : The alert threshold that sets the state of
> + charge level where an interrupt is generated.
> + Can be configured from 1 up to 32. If skipped
> + the power up default value of 4 will be used.
Units? This is a low or high alert? Does a common property make sense
here?
> +- interrupt-parent : The GPIO bank from the interrupt line.
Drop this. interrupt-parent is implied.
> +- interrupts : Interrupt line see Documentation/devicetree/
> + bindings/interrupt-controller/interrupts.txt
> +
> +Example:
> +
> + battery-charger@36 {
> + compatible = "maxim,max17040";
> + reg = <0x36>;
> + maxim,alert-soc-level = <10>;
> + interrupt-parent = <&gpio7>;
> + interrupts = <2 IRQ_TYPE_EDGE_FALLING>;
Usually there are battery properties that need to be described too...
> + };
> --
> 2.17.0
>
On 4/29/19 7:13 PM, Rob Herring wrote:
> On Sun, Apr 14, 2019 at 10:26:33PM -0300, Matheus Castello wrote:
>> For configure low level state of charge threshold alert signaled from
>> max17040 we add "maxim,alert-soc-level" property.
>>
>> Signed-off-by: Matheus Castello <[email protected]>
>> ---
>> .../power/supply/max17040_battery.txt | 24 +++++++++++++++++++
>> 1 file changed, 24 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt
>>
>> diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
>> new file mode 100644
>> index 000000000000..9b2cc67d556f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
>> @@ -0,0 +1,24 @@
>> +max17040_battery
>> +~~~~~~~~~~~~~~~~
>> +
>> +Required properties :
>> + - compatible : "maxim,max17040" or "maxim,max77836-battery"
>
> This is really a charger, not a battery.
>
max17040 is a fuel gauge, max77836 MUIC has it integrated. Because of
this we use it in the compatible list.
>> +
>> +Optional properties :
>> +- maxim,alert-soc-level : The alert threshold that sets the state of
>> + charge level where an interrupt is generated.
>> + Can be configured from 1 up to 32. If skipped
>> + the power up default value of 4 will be used.
>
> Units? This is a low or high alert? Does a common property make sense
> here?
>
It is a low level alert. I will change the name of the property to
"maxim,alert-low-soc-level" to make this clear and I will put the
percent unit in the description.
I do not find any common property that I can use here, if I am wrong let
me know.
>> +- interrupt-parent : The GPIO bank from the interrupt line.
>
> Drop this. interrupt-parent is implied.
Ok, I will do.
>
>> +- interrupts : Interrupt line see Documentation/devicetree/
>> + bindings/interrupt-controller/interrupts.txt
>> +
>> +Example:
>> +
>> + battery-charger@36 {
>> + compatible = "maxim,max17040";
>> + reg = <0x36>;
>> + maxim,alert-soc-level = <10>;
>> + interrupt-parent = <&gpio7>;
>> + interrupts = <2 IRQ_TYPE_EDGE_FALLING>;
>
> Usually there are battery properties that need to be described too...
>
I will fix this for "battery-fuel-gauge@36". I will also add the
description for wake-source as optional property.
Thanks.
Best Regards,
Matheus Castello
>> + };
>> --
>> 2.17.0
>>
This series add IRQ handler for low level SOC alert, define a devicetree
binding attribute to configure the alert level threshold and check for
changes in SOC and power supply status for send uevents.
Max17040 have a pin for alert host about low level state of charge and
this alert can be configured in a threshold from 1% up to 32% of SOC.
Tested on Toradex Colibri iMX7D, with a SparkFun Lipo Fuel Gauge module
based on MAXIM MAX17043.
Thanks Krzysztof and Rob for yours time reviewing it. Let me know what
you think about the fixes.
Krzysztof I added a new patch to the series to check the battery charge up
and clear ALRT bit when the SOC is above alert threshold, so not generating
duplicate interrupts.
Changes since v1:
(Suggested by Krzysztof Kozlowski)
- Put common code from max17040_work and max17040_thread_handler in a function
- Code style fixes
- Define mask and low level threshold alert default
- Check return value from max17040_set_soc_threshold
- Set low level state of charge threshold before IRQ
- CC maintainers from drivers/mfd/max14577
- Use flags from FDT client->flags instead hard coded it
- Mention interrupts on DT Documentation
- Fix "maxim,max77836-battery" missed from DT Documentation
- Fix commit description
Changes since v2:
(Suggested by Krzysztof Kozlowski)
- Fix ebusy exception
- Remove device_init_wakeup from probe, let wakeup-source from DT decide that
- Fix the use of "charger" to fuel-gauge on device tree description
- Clear ALRT bit while setting the SOC threshold
- Check SOC and clear ALRT bit when the SOC are above alert threshold
- Fix unnecessary uevent when SOC is an ERRNO
- Notify user space when power supply status change
(Suggested by Rob Herring)
- Fix the use of "charger" to fuel-gauge on device tree description
- Add the (%) units on the description of property
- Drop interrupt-parent
- Fix name of property to let clear that is a low level SOC alert
Matheus Castello (5):
power: supply: max17040: Add IRQ handler for low SOC alert
dt-bindings: power: supply: Max17040: Add low level SOC alert
threshold
power: supply: max17040: Config alert SOC low level threshold from FDT
power: supply: max17040: Clear ALRT bit when the SOC are above
threshold
power: supply: max17040: Send uevent in SOC and status change
.../power/supply/max17040_battery.txt | 28 ++++
drivers/power/supply/max17040_battery.c | 134 +++++++++++++++++-
2 files changed, 158 insertions(+), 4 deletions(-)
create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt
--
2.20.1
For configuration of fuel gauge alert for a low level state of charge
interrupt we add a function to config level threshold and a device tree
binding property to set it in flatned device tree node.
Now we can use "maxim,alert-low-soc-level" property with the values from
1% up to 32% to configure alert interrupt threshold.
Signed-off-by: Matheus Castello <[email protected]>
---
drivers/power/supply/max17040_battery.c | 52 +++++++++++++++++++++++--
1 file changed, 49 insertions(+), 3 deletions(-)
diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
index b7433e9ca7c2..2f4851608cfe 100644
--- a/drivers/power/supply/max17040_battery.c
+++ b/drivers/power/supply/max17040_battery.c
@@ -29,6 +29,9 @@
#define MAX17040_DELAY 1000
#define MAX17040_BATTERY_FULL 95
+#define MAX17040_ATHD_MASK 0xFFC0
+#define MAX17040_ATHD_DEFAULT_POWER_UP 4
+
struct max17040_chip {
struct i2c_client *client;
struct delayed_work work;
@@ -43,6 +46,8 @@ struct max17040_chip {
int soc;
/* State Of Charge */
int status;
+ /* Low alert threshold from 32% to 1% of the State of Charge */
+ u32 low_soc_alert_threshold;
};
static int max17040_get_property(struct power_supply *psy,
@@ -99,6 +104,28 @@ static void max17040_reset(struct i2c_client *client)
max17040_write_reg(client, MAX17040_CMD, 0x0054);
}
+static int max17040_set_low_soc_threshold_alert(struct i2c_client *client,
+ u32 level)
+{
+ int ret;
+ u16 data;
+
+ /* check if level is between 1% and 32% */
+ if (level > 0 && level < 33) {
+ level = 32 - level;
+ data = max17040_read_reg(client, MAX17040_RCOMP);
+ /* clear the alrt bit and set LSb 5 bits */
+ data &= MAX17040_ATHD_MASK;
+ data |= level;
+ max17040_write_reg(client, MAX17040_RCOMP, data);
+ ret = 0;
+ } else {
+ ret = -EINVAL;
+ }
+
+ return ret;
+}
+
static void max17040_get_vcell(struct i2c_client *client)
{
struct max17040_chip *chip = i2c_get_clientdata(client);
@@ -161,6 +188,16 @@ static void max17040_get_status(struct i2c_client *client)
chip->status = POWER_SUPPLY_STATUS_FULL;
}
+static void max17040_get_of_data(struct max17040_chip *chip)
+{
+ struct device *dev = &chip->client->dev;
+ struct device_node *np = dev->of_node;
+
+ if (of_property_read_u32(np, "maxim,alert-low-soc-level",
+ &chip->low_soc_alert_threshold))
+ chip->low_soc_alert_threshold = MAX17040_ATHD_DEFAULT_POWER_UP;
+}
+
static void max17040_check_changes(struct i2c_client *client)
{
max17040_get_vcell(client);
@@ -226,6 +263,7 @@ static int max17040_probe(struct i2c_client *client,
chip->client = client;
chip->pdata = client->dev.platform_data;
+ max17040_get_of_data(chip);
i2c_set_clientdata(client, chip);
psy_cfg.drv_data = chip;
@@ -243,12 +281,20 @@ static int max17040_probe(struct i2c_client *client,
/* check interrupt */
if (client->irq) {
int ret;
- unsigned int flags;
+
+ ret = max17040_set_low_soc_threshold_alert(client,
+ chip->low_soc_alert_threshold);
+ if (ret) {
+ dev_err(&client->dev,
+ "Failed to set low SOC alert: err %d\n",
+ ret);
+ return ret;
+ }
dev_info(&client->dev, "IRQ: enabled\n");
- flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT;
ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
- max17040_thread_handler, flags,
+ max17040_thread_handler,
+ (client->flags | IRQF_ONESHOT),
chip->battery->desc->name,
chip);
--
2.20.1
For configure low level state of charge threshold alert signaled from
max17040 we add "maxim,alert-low-soc-level" property.
Signed-off-by: Matheus Castello <[email protected]>
---
.../power/supply/max17040_battery.txt | 28 +++++++++++++++++++
1 file changed, 28 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt
diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
new file mode 100644
index 000000000000..a13e8d50ff7b
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
@@ -0,0 +1,28 @@
+max17040_battery
+~~~~~~~~~~~~~~~~
+
+Required properties :
+ - compatible : "maxim,max17040" or "maxim,max77836-battery"
+
+Optional properties :
+- maxim,alert-low-soc-level : The alert threshold that sets the state of
+ charge level (%) where an interrupt is
+ generated. Can be configured from 1 up to 32
+ (%). If skipped the power up default value of
+ 4 (%) will be used.
+- interrupts : Interrupt line see Documentation/devicetree/
+ bindings/interrupt-controller/interrupts.txt
+- wakeup-source : This device has wakeup capabilities. Use this
+ property to use alert low SOC level interrupt
+ as wake up source.
+
+Example:
+
+ battery-fuel-gauge@36 {
+ compatible = "maxim,max17040";
+ reg = <0x36>;
+ maxim,alert-low-soc-level = <10>;
+ interrupt-parent = <&gpio7>;
+ interrupts = <2 IRQ_TYPE_EDGE_FALLING>;
+ wakeup-source;
+ };
--
2.20.1
Notify core through power_supply_changed() in case of changes in state
of charge and power supply status. This is useful for user-space to
efficiently update current battery level.
Signed-off-by: Matheus Castello <[email protected]>
---
drivers/power/supply/max17040_battery.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
index 61e6fcfea8a1..34278845cfe5 100644
--- a/drivers/power/supply/max17040_battery.c
+++ b/drivers/power/supply/max17040_battery.c
@@ -176,6 +176,9 @@ static void max17040_get_online(struct i2c_client *client)
static void max17040_get_status(struct i2c_client *client)
{
struct max17040_chip *chip = i2c_get_clientdata(client);
+ int last_status;
+
+ last_status = chip->status;
if (!chip->pdata || !chip->pdata->charger_online
|| !chip->pdata->charger_enable) {
@@ -194,6 +197,9 @@ static void max17040_get_status(struct i2c_client *client)
if (chip->soc > MAX17040_BATTERY_FULL)
chip->status = POWER_SUPPLY_STATUS_FULL;
+
+ if (last_status != chip->status)
+ power_supply_changed(chip->battery);
}
static void max17040_get_of_data(struct max17040_chip *chip)
@@ -217,10 +223,18 @@ static void max17040_check_changes(struct i2c_client *client)
static void max17040_work(struct work_struct *work)
{
struct max17040_chip *chip;
+ int last_soc;
chip = container_of(work, struct max17040_chip, work.work);
+
+ /* store SOC for check change */
+ last_soc = chip->soc;
max17040_check_changes(chip->client);
+ /* check changes and send uevent */
+ if (chip->soc >= 0 && last_soc != chip->soc)
+ power_supply_changed(chip->battery);
+
queue_delayed_work(system_power_efficient_wq, &chip->work,
MAX17040_DELAY);
}
--
2.20.1
According datasheet max17040 has a pin for alert host for low SOC.
This pin can be used as external interrupt, so we need to check for
interrupts assigned for device and handle it.
In handler we are checking and storing fuel gauge registers values
and send an uevent to notificate user space, so user space can decide
save work or turn off since the alert demonstrate that the battery may
no have the power to keep the system turned on for much longer.
Signed-off-by: Matheus Castello <[email protected]>
---
drivers/power/supply/max17040_battery.c | 65 +++++++++++++++++++++++--
1 file changed, 60 insertions(+), 5 deletions(-)
diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
index 91cafc7bed30..b7433e9ca7c2 100644
--- a/drivers/power/supply/max17040_battery.c
+++ b/drivers/power/supply/max17040_battery.c
@@ -13,6 +13,7 @@
#include <linux/err.h>
#include <linux/i2c.h>
#include <linux/delay.h>
+#include <linux/interrupt.h>
#include <linux/power_supply.h>
#include <linux/max17040_battery.h>
#include <linux/slab.h>
@@ -160,21 +161,40 @@ static void max17040_get_status(struct i2c_client *client)
chip->status = POWER_SUPPLY_STATUS_FULL;
}
+static void max17040_check_changes(struct i2c_client *client)
+{
+ max17040_get_vcell(client);
+ max17040_get_soc(client);
+ max17040_get_online(client);
+ max17040_get_status(client);
+}
+
static void max17040_work(struct work_struct *work)
{
struct max17040_chip *chip;
chip = container_of(work, struct max17040_chip, work.work);
-
- max17040_get_vcell(chip->client);
- max17040_get_soc(chip->client);
- max17040_get_online(chip->client);
- max17040_get_status(chip->client);
+ max17040_check_changes(chip->client);
queue_delayed_work(system_power_efficient_wq, &chip->work,
MAX17040_DELAY);
}
+static irqreturn_t max17040_thread_handler(int id, void *dev)
+{
+ struct max17040_chip *chip = dev;
+ struct i2c_client *client = chip->client;
+
+ dev_warn(&client->dev, "IRQ: Alert battery low level");
+ /* read registers */
+ max17040_check_changes(chip->client);
+
+ /* send uevent */
+ power_supply_changed(chip->battery);
+
+ return IRQ_HANDLED;
+}
+
static enum power_supply_property max17040_battery_props[] = {
POWER_SUPPLY_PROP_STATUS,
POWER_SUPPLY_PROP_ONLINE,
@@ -220,6 +240,25 @@ static int max17040_probe(struct i2c_client *client,
max17040_reset(client);
max17040_get_version(client);
+ /* check interrupt */
+ if (client->irq) {
+ int ret;
+ unsigned int flags;
+
+ dev_info(&client->dev, "IRQ: enabled\n");
+ flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT;
+ ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
+ max17040_thread_handler, flags,
+ chip->battery->desc->name,
+ chip);
+
+ if (ret) {
+ client->irq = 0;
+ dev_warn(&client->dev,
+ "Failed to get IRQ err %d\n", ret);
+ }
+ }
+
INIT_DEFERRABLE_WORK(&chip->work, max17040_work);
queue_delayed_work(system_power_efficient_wq, &chip->work,
MAX17040_DELAY);
@@ -244,6 +283,14 @@ static int max17040_suspend(struct device *dev)
struct max17040_chip *chip = i2c_get_clientdata(client);
cancel_delayed_work(&chip->work);
+
+ if (client->irq) {
+ if (device_may_wakeup(dev))
+ enable_irq_wake(client->irq);
+ else
+ disable_irq_wake(client->irq);
+ }
+
return 0;
}
@@ -254,6 +301,14 @@ static int max17040_resume(struct device *dev)
queue_delayed_work(system_power_efficient_wq, &chip->work,
MAX17040_DELAY);
+
+ if (client->irq) {
+ if (device_may_wakeup(dev))
+ disable_irq_wake(client->irq);
+ else
+ enable_irq_wake(client->irq);
+ }
+
return 0;
}
--
2.20.1
On Mon, 27 May 2019 at 04:45, Matheus Castello <[email protected]> wrote:
>
> According datasheet max17040 has a pin for alert host for low SOC.
> This pin can be used as external interrupt, so we need to check for
> interrupts assigned for device and handle it.
>
> In handler we are checking and storing fuel gauge registers values
> and send an uevent to notificate user space, so user space can decide
> save work or turn off since the alert demonstrate that the battery may
> no have the power to keep the system turned on for much longer.
>
> Signed-off-by: Matheus Castello <[email protected]>
> ---
> drivers/power/supply/max17040_battery.c | 65 +++++++++++++++++++++++--
> 1 file changed, 60 insertions(+), 5 deletions(-)
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On Mon, 27 May 2019 at 04:45, Matheus Castello <[email protected]> wrote:
>
> For configure low level state of charge threshold alert signaled from
> max17040 we add "maxim,alert-low-soc-level" property.
>
> Signed-off-by: Matheus Castello <[email protected]>
> ---
> .../power/supply/max17040_battery.txt | 28 +++++++++++++++++++
> 1 file changed, 28 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt
>
> diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
> new file mode 100644
> index 000000000000..a13e8d50ff7b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
> @@ -0,0 +1,28 @@
> +max17040_battery
> +~~~~~~~~~~~~~~~~
> +
> +Required properties :
> + - compatible : "maxim,max17040" or "maxim,max77836-battery"
> +
> +Optional properties :
> +- maxim,alert-low-soc-level : The alert threshold that sets the state of
> + charge level (%) where an interrupt is
> + generated. Can be configured from 1 up to 32
> + (%). If skipped the power up default value of
> + 4 (%) will be used.
> +- interrupts : Interrupt line see Documentation/devicetree/
> + bindings/interrupt-controller/interrupts.txt
Based on driver's behavior, I understand that these two properties
come in pair so maxim,alert-low-soc-level (or its default value) will
be used if and only if interrupts property is present. Maybe mention
this? In general looks fine to me:
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
> +- wakeup-source : This device has wakeup capabilities. Use this
> + property to use alert low SOC level interrupt
> + as wake up source.
> +
> +Example:
> +
> + battery-fuel-gauge@36 {
> + compatible = "maxim,max17040";
> + reg = <0x36>;
> + maxim,alert-low-soc-level = <10>;
> + interrupt-parent = <&gpio7>;
> + interrupts = <2 IRQ_TYPE_EDGE_FALLING>;
> + wakeup-source;
> + };
> --
> 2.20.1
>
On Mon, 27 May 2019 at 04:46, Matheus Castello <[email protected]> wrote:
>
> For configuration of fuel gauge alert for a low level state of charge
> interrupt we add a function to config level threshold and a device tree
> binding property to set it in flatned device tree node.
>
> Now we can use "maxim,alert-low-soc-level" property with the values from
> 1% up to 32% to configure alert interrupt threshold.
>
> Signed-off-by: Matheus Castello <[email protected]>
> ---
> drivers/power/supply/max17040_battery.c | 52 +++++++++++++++++++++++--
> 1 file changed, 49 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
> index b7433e9ca7c2..2f4851608cfe 100644
> --- a/drivers/power/supply/max17040_battery.c
> +++ b/drivers/power/supply/max17040_battery.c
> @@ -29,6 +29,9 @@
> #define MAX17040_DELAY 1000
> #define MAX17040_BATTERY_FULL 95
>
> +#define MAX17040_ATHD_MASK 0xFFC0
> +#define MAX17040_ATHD_DEFAULT_POWER_UP 4
> +
> struct max17040_chip {
> struct i2c_client *client;
> struct delayed_work work;
> @@ -43,6 +46,8 @@ struct max17040_chip {
> int soc;
> /* State Of Charge */
> int status;
> + /* Low alert threshold from 32% to 1% of the State of Charge */
> + u32 low_soc_alert_threshold;
> };
>
> static int max17040_get_property(struct power_supply *psy,
> @@ -99,6 +104,28 @@ static void max17040_reset(struct i2c_client *client)
> max17040_write_reg(client, MAX17040_CMD, 0x0054);
> }
>
> +static int max17040_set_low_soc_threshold_alert(struct i2c_client *client,
> + u32 level)
> +{
> + int ret;
> + u16 data;
> +
> + /* check if level is between 1% and 32% */
> + if (level > 0 && level < 33) {
> + level = 32 - level;
> + data = max17040_read_reg(client, MAX17040_RCOMP);
> + /* clear the alrt bit and set LSb 5 bits */
> + data &= MAX17040_ATHD_MASK;
> + data |= level;
> + max17040_write_reg(client, MAX17040_RCOMP, data);
> + ret = 0;
> + } else {
> + ret = -EINVAL;
> + }
This is unusual way of handling error... when you parse DTS, you
accept any value for "level" (even incorrect one). You validate the
value later when setting it and show an error... however you ignore
the error of max17040_write_reg() here... This is correct but looks
unusual.
Best regards,
Krzysztof
On Mon, 27 May 2019 at 04:45, Matheus Castello <[email protected]> wrote:
>
> For configure low level state of charge threshold alert signaled from
> max17040 we add "maxim,alert-low-soc-level" property.
>
> Signed-off-by: Matheus Castello <[email protected]>
> ---
> .../power/supply/max17040_battery.txt | 28 +++++++++++++++++++
> 1 file changed, 28 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt
>
> diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
> new file mode 100644
> index 000000000000..a13e8d50ff7b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
> @@ -0,0 +1,28 @@
> +max17040_battery
> +~~~~~~~~~~~~~~~~
> +
> +Required properties :
> + - compatible : "maxim,max17040" or "maxim,max77836-battery"
One more comment. The datasheet for max17040 says that there is on
ALERT pin and ALERT bits in RCOMP register. Which device are you
using? If it turns out that max17040 does not support it, then the
driver and bindings should reflect this - interrupts should not be set
on max17040.
Best regards,
Krzysztof
On Mon, 27 May 2019 at 04:23, Matheus Castello <[email protected]> wrote:
>
> Notify core through power_supply_changed() in case of changes in state
> of charge and power supply status. This is useful for user-space to
> efficiently update current battery level.
>
> Signed-off-by: Matheus Castello <[email protected]>
> ---
> drivers/power/supply/max17040_battery.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
> index 61e6fcfea8a1..34278845cfe5 100644
> --- a/drivers/power/supply/max17040_battery.c
> +++ b/drivers/power/supply/max17040_battery.c
> @@ -176,6 +176,9 @@ static void max17040_get_online(struct i2c_client *client)
> static void max17040_get_status(struct i2c_client *client)
> {
> struct max17040_chip *chip = i2c_get_clientdata(client);
> + int last_status;
> +
> + last_status = chip->status;
>
> if (!chip->pdata || !chip->pdata->charger_online
> || !chip->pdata->charger_enable) {
> @@ -194,6 +197,9 @@ static void max17040_get_status(struct i2c_client *client)
>
> if (chip->soc > MAX17040_BATTERY_FULL)
> chip->status = POWER_SUPPLY_STATUS_FULL;
> +
> + if (last_status != chip->status)
> + power_supply_changed(chip->battery);
Why splitting it from max17040_work()? It seems logical to check soc
and status at the same time.
Best regards,
Krzysztof
> }
>
> static void max17040_get_of_data(struct max17040_chip *chip)
> @@ -217,10 +223,18 @@ static void max17040_check_changes(struct i2c_client *client)
> static void max17040_work(struct work_struct *work)
> {
> struct max17040_chip *chip;
> + int last_soc;
>
> chip = container_of(work, struct max17040_chip, work.work);
> +
> + /* store SOC for check change */
> + last_soc = chip->soc;
> max17040_check_changes(chip->client);
>
> + /* check changes and send uevent */
> + if (chip->soc >= 0 && last_soc != chip->soc)
> + power_supply_changed(chip->battery);
> +
> queue_delayed_work(system_power_efficient_wq, &chip->work,
> MAX17040_DELAY);
> }
> --
> 2.20.1
>
> On Mon, 27 May 2019 at 04:45, Matheus Castello <[email protected]> wrote:
>>
>> For configure low level state of charge threshold alert signaled from
>> max17040 we add "maxim,alert-low-soc-level" property.
>>
>> Signed-off-by: Matheus Castello <[email protected]>
>> ---
>> .../power/supply/max17040_battery.txt | 28 +++++++++++++++++++
>> 1 file changed, 28 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt
>>
>> diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
>> new file mode 100644
>> index 000000000000..a13e8d50ff7b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
>> @@ -0,0 +1,28 @@
>> +max17040_battery
>> +~~~~~~~~~~~~~~~~
>> +
>> +Required properties :
>> + - compatible : "maxim,max17040" or "maxim,max77836-battery"
>
> One more comment. The datasheet for max17040 says that there is on
> ALERT pin and ALERT bits in RCOMP register. Which device are you
> using? If it turns out that max17040 does not support it, then the
> driver and bindings should reflect this - interrupts should not be set
> on max17040.
>
Yes you are right, max17040 have no ALERT pin. I am using max17043. Let
me know what you think would be best, put a note about it in the
description, add a compatibles like "maxim,max17043" and
"maxim,max17044"? What do you think?
Best Regards,
Matheus Castello
> Best regards,
> Krzysztof
>
On 5/29/19 11:46 AM, Krzysztof Kozlowski wrote:
> On Mon, 27 May 2019 at 04:46, Matheus Castello <[email protected]> wrote:
>>
>> For configuration of fuel gauge alert for a low level state of charge
>> interrupt we add a function to config level threshold and a device tree
>> binding property to set it in flatned device tree node.
>>
>> Now we can use "maxim,alert-low-soc-level" property with the values from
>> 1% up to 32% to configure alert interrupt threshold.
>>
>> Signed-off-by: Matheus Castello <[email protected]>
>> ---
>> drivers/power/supply/max17040_battery.c | 52 +++++++++++++++++++++++--
>> 1 file changed, 49 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
>> index b7433e9ca7c2..2f4851608cfe 100644
>> --- a/drivers/power/supply/max17040_battery.c
>> +++ b/drivers/power/supply/max17040_battery.c
>> @@ -29,6 +29,9 @@
>> #define MAX17040_DELAY 1000
>> #define MAX17040_BATTERY_FULL 95
>>
>> +#define MAX17040_ATHD_MASK 0xFFC0
>> +#define MAX17040_ATHD_DEFAULT_POWER_UP 4
>> +
>> struct max17040_chip {
>> struct i2c_client *client;
>> struct delayed_work work;
>> @@ -43,6 +46,8 @@ struct max17040_chip {
>> int soc;
>> /* State Of Charge */
>> int status;
>> + /* Low alert threshold from 32% to 1% of the State of Charge */
>> + u32 low_soc_alert_threshold;
>> };
>>
>> static int max17040_get_property(struct power_supply *psy,
>> @@ -99,6 +104,28 @@ static void max17040_reset(struct i2c_client *client)
>> max17040_write_reg(client, MAX17040_CMD, 0x0054);
>> }
>>
>> +static int max17040_set_low_soc_threshold_alert(struct i2c_client *client,
>> + u32 level)
>> +{
>> + int ret;
>> + u16 data;
>> +
>> + /* check if level is between 1% and 32% */
>> + if (level > 0 && level < 33) {
>> + level = 32 - level;
>> + data = max17040_read_reg(client, MAX17040_RCOMP);
>> + /* clear the alrt bit and set LSb 5 bits */
>> + data &= MAX17040_ATHD_MASK;
>> + data |= level;
>> + max17040_write_reg(client, MAX17040_RCOMP, data);
>> + ret = 0;
I will put the return of max17040_write_reg on ret, instead of ret = 0.
>> + } else {
>> + ret = -EINVAL;
>> + }
>
> This is unusual way of handling error... when you parse DTS, you
> accept any value for "level" (even incorrect one). You validate the
> value later when setting it and show an error... however you ignore
> the error of max17040_write_reg() here... This is correct but looks
> unusual.
>
Ok, so would it be better to check the level value in
"max17040_get_of_data" and return an error there if the input is wrong?
Best Regards,
Matheus Castello
> Best regards,
> Krzysztof
>
On Sun, 2 Jun 2019 at 23:42, Matheus Castello <[email protected]> wrote:
>
> > On Mon, 27 May 2019 at 04:45, Matheus Castello <[email protected]> wrote:
> >>
> >> For configure low level state of charge threshold alert signaled from
> >> max17040 we add "maxim,alert-low-soc-level" property.
> >>
> >> Signed-off-by: Matheus Castello <[email protected]>
> >> ---
> >> .../power/supply/max17040_battery.txt | 28 +++++++++++++++++++
> >> 1 file changed, 28 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
> >> new file mode 100644
> >> index 000000000000..a13e8d50ff7b
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
> >> @@ -0,0 +1,28 @@
> >> +max17040_battery
> >> +~~~~~~~~~~~~~~~~
> >> +
> >> +Required properties :
> >> + - compatible : "maxim,max17040" or "maxim,max77836-battery"
> >
> > One more comment. The datasheet for max17040 says that there is on
> > ALERT pin and ALERT bits in RCOMP register. Which device are you
> > using? If it turns out that max17040 does not support it, then the
> > driver and bindings should reflect this - interrupts should not be set
> > on max17040.
> >
>
> Yes you are right, max17040 have no ALERT pin. I am using max17043. Let
> me know what you think would be best, put a note about it in the
> description, add a compatibles like "maxim,max17043" and
> "maxim,max17044"? What do you think?
Usually in such case driver should behave differently for different
devices. This difference is chosen by compatible. Since there is
already max77836 compatible - which has the ALERT pin (probably it
just includes 17043 fuel gauge) - you can customize it. No need for
new compatible, unless it stops working on max77836...
Anyway, configuring interrupts on max17040 would be probably harmless
but still it is not really correct. The registers should not be
touched in such case.
Best regards,
Krzysztof
On Mon, 3 Jun 2019 at 00:42, Matheus Castello <[email protected]> wrote:
>
>
>
> On 5/29/19 11:46 AM, Krzysztof Kozlowski wrote:
> > On Mon, 27 May 2019 at 04:46, Matheus Castello <[email protected]> wrote:
> >>
> >> For configuration of fuel gauge alert for a low level state of charge
> >> interrupt we add a function to config level threshold and a device tree
> >> binding property to set it in flatned device tree node.
> >>
> >> Now we can use "maxim,alert-low-soc-level" property with the values from
> >> 1% up to 32% to configure alert interrupt threshold.
> >>
> >> Signed-off-by: Matheus Castello <[email protected]>
> >> ---
> >> drivers/power/supply/max17040_battery.c | 52 +++++++++++++++++++++++--
> >> 1 file changed, 49 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
> >> index b7433e9ca7c2..2f4851608cfe 100644
> >> --- a/drivers/power/supply/max17040_battery.c
> >> +++ b/drivers/power/supply/max17040_battery.c
> >> @@ -29,6 +29,9 @@
> >> #define MAX17040_DELAY 1000
> >> #define MAX17040_BATTERY_FULL 95
> >>
> >> +#define MAX17040_ATHD_MASK 0xFFC0
> >> +#define MAX17040_ATHD_DEFAULT_POWER_UP 4
> >> +
> >> struct max17040_chip {
> >> struct i2c_client *client;
> >> struct delayed_work work;
> >> @@ -43,6 +46,8 @@ struct max17040_chip {
> >> int soc;
> >> /* State Of Charge */
> >> int status;
> >> + /* Low alert threshold from 32% to 1% of the State of Charge */
> >> + u32 low_soc_alert_threshold;
> >> };
> >>
> >> static int max17040_get_property(struct power_supply *psy,
> >> @@ -99,6 +104,28 @@ static void max17040_reset(struct i2c_client *client)
> >> max17040_write_reg(client, MAX17040_CMD, 0x0054);
> >> }
> >>
> >> +static int max17040_set_low_soc_threshold_alert(struct i2c_client *client,
> >> + u32 level)
> >> +{
> >> + int ret;
> >> + u16 data;
> >> +
> >> + /* check if level is between 1% and 32% */
> >> + if (level > 0 && level < 33) {
> >> + level = 32 - level;
> >> + data = max17040_read_reg(client, MAX17040_RCOMP);
> >> + /* clear the alrt bit and set LSb 5 bits */
> >> + data &= MAX17040_ATHD_MASK;
> >> + data |= level;
> >> + max17040_write_reg(client, MAX17040_RCOMP, data);
> >> + ret = 0;
>
> I will put the return of max17040_write_reg on ret, instead of ret = 0.
>
> >> + } else {
> >> + ret = -EINVAL;
> >> + }
> >
> > This is unusual way of handling error... when you parse DTS, you
> > accept any value for "level" (even incorrect one). You validate the
> > value later when setting it and show an error... however you ignore
> > the error of max17040_write_reg() here... This is correct but looks
> > unusual.
> >
>
> Ok, so would it be better to check the level value in
> "max17040_get_of_data" and return an error there if the input is wrong?
I think yes. It looks more natural - validate the value as early as
possible and fail the probe which gives the information about point of
failure.
Best regards,
Krzysztof
This series add IRQ handler for low level SOC alert, define a devicetree
binding attribute to configure the alert level threshold and check for
changes in SOC and power supply status for send uevents.
Max17043/17044 have a pin for alert host about low level state of charge and
this alert can be configured in a threshold from 1% up to 32% of SOC.
Tested on Toradex Colibri iMX7D, with a SparkFun Lipo Fuel Gauge module
based on MAXIM MAX17043.
Thanks Krzysztof for your time reviewing it. Let me know what you think about
the fixes.
Changes since v3:
(Suggested by Krzysztof Kozlowski)
- Validate values of the maxim,alert-low-soc-level early as possible
- Clear the alert bit while handling IRQ
- Remove the "chip->alert_bit"
- Check the SOC and status at the same time on max17040_work
- Add note about the compatible of devices with ALERT feature
- Only set alert register and IRQ when compatible is max77836
Changes since v2:
(Suggested by Krzysztof Kozlowski)
- Fix ebusy exception
- Remove device_init_wakeup from probe, let wakeup-source from DT decide that
- Fix the use of "charger" to fuel-gauge on device tree description
- Clear ALRT bit while setting the SOC threshold
- Check SOC and clear ALRT bit when the SOC are above alert threshold
- Fix unnecessary uevent when SOC is an ERRNO
- Notify user space when power supply status change
(Suggested by Rob Herring)
- Fix the use of "charger" to fuel-gauge on device tree description
- Add the (%) units on the description of property
- Drop interrupt-parent
- Fix name of property to let clear that is a low level SOC alert
Changes since v1:
(Suggested by Krzysztof Kozlowski)
- Put common code from max17040_work and max17040_thread_handler in a function
- Code style fixes
- Define mask and low level threshold alert default
- Check return value from max17040_set_soc_threshold
- Set low level state of charge threshold before IRQ
- CC maintainers from drivers/mfd/max14577
- Use flags from FDT client->flags instead hard coded it
- Mention interrupts on DT Documentation
- Fix "maxim,max77836-battery" missed from DT Documentation
- Fix commit description
Matheus Castello (4):
power: supply: max17040: Add IRQ handler for low SOC alert
dt-bindings: power: supply: Max17040: Add low level SOC alert
threshold
power: supply: max17040: Config alert SOC low level threshold from FDT
power: supply: max17040: Send uevent in SOC and status change
.../power/supply/max17040_battery.txt | 33 +++++
drivers/power/supply/max17040_battery.c | 134 +++++++++++++++++-
2 files changed, 162 insertions(+), 5 deletions(-)
create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt
--
2.24.0.rc2
According datasheet max17040 has a pin for alert host for low SOC.
This pin can be used as external interrupt, so we need to check for
interrupts assigned for device and handle it.
In handler we are checking and storing fuel gauge registers values
and send an uevent to notificate user space, so user space can decide
save work or turn off since the alert demonstrate that the battery may
no have the power to keep the system turned on for much longer.
Signed-off-by: Matheus Castello <[email protected]>
---
drivers/power/supply/max17040_battery.c | 65 +++++++++++++++++++++++--
1 file changed, 60 insertions(+), 5 deletions(-)
diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
index 62499018e68b..75459f76d02c 100644
--- a/drivers/power/supply/max17040_battery.c
+++ b/drivers/power/supply/max17040_battery.c
@@ -13,6 +13,7 @@
#include <linux/err.h>
#include <linux/i2c.h>
#include <linux/delay.h>
+#include <linux/interrupt.h>
#include <linux/power_supply.h>
#include <linux/max17040_battery.h>
#include <linux/slab.h>
@@ -160,21 +161,40 @@ static void max17040_get_status(struct i2c_client *client)
chip->status = POWER_SUPPLY_STATUS_FULL;
}
+static void max17040_check_changes(struct i2c_client *client)
+{
+ max17040_get_vcell(client);
+ max17040_get_soc(client);
+ max17040_get_online(client);
+ max17040_get_status(client);
+}
+
static void max17040_work(struct work_struct *work)
{
struct max17040_chip *chip;
chip = container_of(work, struct max17040_chip, work.work);
-
- max17040_get_vcell(chip->client);
- max17040_get_soc(chip->client);
- max17040_get_online(chip->client);
- max17040_get_status(chip->client);
+ max17040_check_changes(chip->client);
queue_delayed_work(system_power_efficient_wq, &chip->work,
MAX17040_DELAY);
}
+static irqreturn_t max17040_thread_handler(int id, void *dev)
+{
+ struct max17040_chip *chip = dev;
+ struct i2c_client *client = chip->client;
+
+ dev_warn(&client->dev, "IRQ: Alert battery low level");
+ /* read registers */
+ max17040_check_changes(chip->client);
+
+ /* send uevent */
+ power_supply_changed(chip->battery);
+
+ return IRQ_HANDLED;
+}
+
static enum power_supply_property max17040_battery_props[] = {
POWER_SUPPLY_PROP_STATUS,
POWER_SUPPLY_PROP_ONLINE,
@@ -220,6 +240,25 @@ static int max17040_probe(struct i2c_client *client,
max17040_reset(client);
max17040_get_version(client);
+ /* check interrupt */
+ if (client->irq) {
+ int ret;
+ unsigned int flags;
+
+ dev_info(&client->dev, "IRQ: enabled\n");
+ flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT;
+ ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
+ max17040_thread_handler, flags,
+ chip->battery->desc->name,
+ chip);
+
+ if (ret) {
+ client->irq = 0;
+ dev_warn(&client->dev,
+ "Failed to get IRQ err %d\n", ret);
+ }
+ }
+
INIT_DEFERRABLE_WORK(&chip->work, max17040_work);
queue_delayed_work(system_power_efficient_wq, &chip->work,
MAX17040_DELAY);
@@ -244,6 +283,14 @@ static int max17040_suspend(struct device *dev)
struct max17040_chip *chip = i2c_get_clientdata(client);
cancel_delayed_work(&chip->work);
+
+ if (client->irq) {
+ if (device_may_wakeup(dev))
+ enable_irq_wake(client->irq);
+ else
+ disable_irq_wake(client->irq);
+ }
+
return 0;
}
@@ -254,6 +301,14 @@ static int max17040_resume(struct device *dev)
queue_delayed_work(system_power_efficient_wq, &chip->work,
MAX17040_DELAY);
+
+ if (client->irq) {
+ if (device_may_wakeup(dev))
+ disable_irq_wake(client->irq);
+ else
+ enable_irq_wake(client->irq);
+ }
+
return 0;
}
--
2.24.0.rc2
On Thu, Oct 31, 2019 at 03:41:31PM -0300, Matheus Castello wrote:
> According datasheet max17040 has a pin for alert host for low SOC.
> This pin can be used as external interrupt, so we need to check for
> interrupts assigned for device and handle it.
>
> In handler we are checking and storing fuel gauge registers values
> and send an uevent to notificate user space, so user space can decide
> save work or turn off since the alert demonstrate that the battery may
> no have the power to keep the system turned on for much longer.
>
> Signed-off-by: Matheus Castello <[email protected]>
> ---
> drivers/power/supply/max17040_battery.c | 65 +++++++++++++++++++++++--
> 1 file changed, 60 insertions(+), 5 deletions(-)
This was already reviewed by me. Where's my tag?
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof