[PATCH 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes

[PATCH 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes

[Matheus Castello]

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

[PATCH 1/4] power: supply: max17040: Add IRQ handler for low SOC alert

[Matheus Castello]

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 <matheus@castello.eng.br>
---
 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

Re: [PATCH 1/4] power: supply: max17040: Add IRQ handler for low SOC alert

[Krzysztof Kozlowski]

On 23 July 2018 at 06:08, Matheus Castello <matheus@castello.eng.br> 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 <matheus@castello.eng.br>
> ---
>  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
>

[PATCH 2/4] power: supply: max17040: Config alert SOC low level threshold from FDT

[Matheus Castello]

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 <matheus@castello.eng.br>
---
 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

Re: [PATCH 2/4] power: supply: max17040: Config alert SOC low level threshold from FDT

[Krzysztof Kozlowski]

On 23 July 2018 at 06:08, Matheus Castello <matheus@castello.eng.br> 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 <matheus@castello.eng.br>
> ---
>  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
>

[PATCH 3/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold

[Matheus Castello]

For configure low level state of charge threshold alert signaled from
max17040 we add "maxim,alert-soc-level" property.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 .../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

Re: [PATCH 3/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold

[Krzysztof Kozlowski]

On 23 July 2018 at 06:08, Matheus Castello <matheus@castello.eng.br> 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 <matheus@castello.eng.br>
> ---
>  .../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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] power: supply: max17040: Send uevent in SOC changes

[Matheus Castello]

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 <matheus@castello.eng.br>
---
 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

Re: [PATCH 4/4] power: supply: max17040: Send uevent in SOC changes

[Krzysztof Kozlowski]

On 23 July 2018 at 06:08, Matheus Castello <matheus@castello.eng.br> 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 <krzk@kernel.org>

Best regards,
Krzysztof


> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> ---
>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes

[Sebastian Reichel]

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

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
> 

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

Re: [PATCH 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes

[Krzysztof Kozlowski]

On Sun, 16 Sep 2018 at 22:03, Sebastian Reichel
<sebastian.reichel@collabora.com> 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

Re: [PATCH 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes

[Matheus Castello]

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
> <sebastian.reichel@collabora.com> 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
> 

[PATCH v2 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes

[Matheus Castello]

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

[PATCH v2 1/4] power: supply: max17040: Add IRQ handler for low SOC alert

[Matheus Castello]

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 <matheus@castello.eng.br>
---
 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

Re: [PATCH v2 1/4] power: supply: max17040: Add IRQ handler for low SOC alert

[Krzysztof Kozlowski]

On Mon, 15 Apr 2019 at 03:49, Matheus Castello <matheus@castello.eng.br> 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 <matheus@castello.eng.br>
> ---
>  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

Re: [PATCH v2 1/4] power: supply: max17040: Add IRQ handler for low SOC alert

[Matheus Castello]

Em 4/15/19 4:10 AM, Krzysztof Kozlowski escreveu:
> On Mon, 15 Apr 2019 at 03:49, Matheus Castello <matheus@castello.eng.br> 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 <matheus@castello.eng.br>
>> ---
>>   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

[PATCH v2 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold

[Matheus Castello]

For configure low level state of charge threshold alert signaled from
max17040 we add "maxim,alert-soc-level" property.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 .../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

Re: [PATCH v2 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold

[Krzysztof Kozlowski]

On Mon, 15 Apr 2019 at 03:50, Matheus Castello <matheus@castello.eng.br> 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 <matheus@castello.eng.br>
> ---
>  .../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
>

Re: [PATCH v2 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold

[Rob Herring]

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 <matheus@castello.eng.br>
> ---
>  .../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
> 

Re: [PATCH v2 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold

[Matheus Castello]

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 <matheus@castello.eng.br>
>> ---
>>   .../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
>>

[PATCH v2 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT

[Matheus Castello]

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 <matheus@castello.eng.br>
---
 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

Re: [PATCH v2 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT

[Krzysztof Kozlowski]

On Mon, 15 Apr 2019 at 03:51, Matheus Castello <matheus@castello.eng.br> 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 <matheus@castello.eng.br>
> ---
>  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

[PATCH v2 4/4] power: supply: max17040: Send uevent in SOC changes

[Matheus Castello]

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 <matheus@castello.eng.br>
---
 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

Re: [PATCH v2 4/4] power: supply: max17040: Send uevent in SOC changes

[Krzysztof Kozlowski]

On Mon, 15 Apr 2019 at 03:48, Matheus Castello <matheus@castello.eng.br> 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 <matheus@castello.eng.br>
> ---
>  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

[PATCH v3 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes

[Matheus Castello]

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

[PATCH v3 1/5] power: supply: max17040: Add IRQ handler for low SOC alert

[Matheus Castello]

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 <matheus@castello.eng.br>
---
 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

Re: [PATCH v3 1/5] power: supply: max17040: Add IRQ handler for low SOC alert

[Krzysztof Kozlowski]

On Mon, 27 May 2019 at 04:45, Matheus Castello <matheus@castello.eng.br> 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 <matheus@castello.eng.br>
> ---
>  drivers/power/supply/max17040_battery.c | 65 +++++++++++++++++++++++--
>  1 file changed, 60 insertions(+), 5 deletions(-)

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

[PATCH v3 2/5] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold

[Matheus Castello]

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 <matheus@castello.eng.br>
---
 .../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

Re: [PATCH v3 2/5] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold

[Krzysztof Kozlowski]

On Mon, 27 May 2019 at 04:45, Matheus Castello <matheus@castello.eng.br> 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 <matheus@castello.eng.br>
> ---
>  .../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 <krzk@kernel.org>

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
>

Re: [PATCH v3 2/5] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold

[Krzysztof Kozlowski]

On Mon, 27 May 2019 at 04:45, Matheus Castello <matheus@castello.eng.br> 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 <matheus@castello.eng.br>
> ---
>  .../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

Re: [PATCH v3 2/5] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold

[Matheus Castello]

> On Mon, 27 May 2019 at 04:45, Matheus Castello <matheus@castello.eng.br> 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 <matheus@castello.eng.br>
>> ---
>>   .../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
> 

Re: [PATCH v3 2/5] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold

[Krzysztof Kozlowski]

On Sun, 2 Jun 2019 at 23:42, Matheus Castello <matheus@castello.eng.br> wrote:
>
> > On Mon, 27 May 2019 at 04:45, Matheus Castello <matheus@castello.eng.br> 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 <matheus@castello.eng.br>
> >> ---
> >>   .../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

[PATCH v3 3/5] power: supply: max17040: Config alert SOC low level threshold from FDT

[Matheus Castello]

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 <matheus@castello.eng.br>
---
 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

Re: [PATCH v3 3/5] power: supply: max17040: Config alert SOC low level threshold from FDT

[Krzysztof Kozlowski]

On Mon, 27 May 2019 at 04:46, Matheus Castello <matheus@castello.eng.br> 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 <matheus@castello.eng.br>
> ---
>  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

Re: [PATCH v3 3/5] power: supply: max17040: Config alert SOC low level threshold from FDT

[Matheus Castello]

On 5/29/19 11:46 AM, Krzysztof Kozlowski wrote:
> On Mon, 27 May 2019 at 04:46, Matheus Castello <matheus@castello.eng.br> 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 <matheus@castello.eng.br>
>> ---
>>   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
> 

Re: [PATCH v3 3/5] power: supply: max17040: Config alert SOC low level threshold from FDT

[Krzysztof Kozlowski]

On Mon, 3 Jun 2019 at 00:42, Matheus Castello <matheus@castello.eng.br> wrote:
>
>
>
> On 5/29/19 11:46 AM, Krzysztof Kozlowski wrote:
> > On Mon, 27 May 2019 at 04:46, Matheus Castello <matheus@castello.eng.br> 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 <matheus@castello.eng.br>
> >> ---
> >>   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

[PATCH v3 4/5] power: supply: max17040: Clear ALRT bit when the SOC are above threshold

[Matheus Castello]

In order to not generate duplicate interrupts we clear the ALRT bit when
the SOC is in a state that shows that the battery is charged above the set
threshold for the SOC low level alert.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 drivers/power/supply/max17040_battery.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
index 2f4851608cfe..61e6fcfea8a1 100644
--- a/drivers/power/supply/max17040_battery.c
+++ b/drivers/power/supply/max17040_battery.c
@@ -48,6 +48,7 @@ struct max17040_chip {
 	int status;
 	/* Low alert threshold from 32% to 1% of the State of Charge */
 	u32 low_soc_alert_threshold;
+	int alert_bit;
 };

 static int max17040_get_property(struct power_supply *psy,
@@ -107,6 +108,7 @@ static void max17040_reset(struct i2c_client *client)
 static int max17040_set_low_soc_threshold_alert(struct i2c_client *client,
 	u32 level)
 {
+	struct max17040_chip *chip = i2c_get_clientdata(client);
 	int ret;
 	u16 data;

@@ -118,6 +120,7 @@ static int max17040_set_low_soc_threshold_alert(struct i2c_client *client,
 		data &= MAX17040_ATHD_MASK;
 		data |= level;
 		max17040_write_reg(client, MAX17040_RCOMP, data);
+		chip->alert_bit = 0;
 		ret = 0;
 	} else {
 		ret = -EINVAL;
@@ -144,6 +147,11 @@ static void max17040_get_soc(struct i2c_client *client)
 	soc = max17040_read_reg(client, MAX17040_SOC);

 	chip->soc = (soc >> 8);
+
+	/* check SOC level to clear ALRT bit */
+	if (chip->soc > chip->low_soc_alert_threshold && chip->alert_bit)
+		max17040_set_low_soc_threshold_alert(client,
+			chip->low_soc_alert_threshold);
 }

 static void max17040_get_version(struct i2c_client *client)
@@ -229,6 +237,9 @@ static irqreturn_t max17040_thread_handler(int id, void *dev)
 	/* send uevent */
 	power_supply_changed(chip->battery);

+	/* ALRT bit is seted */
+	chip->alert_bit = 1;
+
 	return IRQ_HANDLED;
 }

--
2.20.1

Re: [PATCH v3 4/5] power: supply: max17040: Clear ALRT bit when the SOC are above threshold

[Krzysztof Kozlowski]

On Mon, 27 May 2019 at 04:45, Matheus Castello <matheus@castello.eng.br> wrote:
>
> In order to not generate duplicate interrupts we clear the ALRT bit when
> the SOC is in a state that shows that the battery is charged above the set
> threshold for the SOC low level alert.

I think interrupt/alert bit should be cleared while handling
interrupt, not later because:
1. It is logical to clear it when servicing it,
2. It is simpler - no need for "chip->alert_bit",
3. The alert threshold is understood as alert/warning so every
interrupt should generate uevent. I understand you wanted to remove
"duplicate interrupts" but in fact there are no duplicates. Every next
interrupt comes from change of SoC while being below the critical
level. Therefore on each such change user-space should be woken up and
notified (e.g. to show the message to the user).

I also think this should be squashed with previous patch as it does
not make sense as standalone commit.

>
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> ---
>  drivers/power/supply/max17040_battery.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
> index 2f4851608cfe..61e6fcfea8a1 100644
> --- a/drivers/power/supply/max17040_battery.c
> +++ b/drivers/power/supply/max17040_battery.c
> @@ -48,6 +48,7 @@ struct max17040_chip {
>         int status;
>         /* Low alert threshold from 32% to 1% of the State of Charge */
>         u32 low_soc_alert_threshold;
> +       int alert_bit;
>  };
>
>  static int max17040_get_property(struct power_supply *psy,
> @@ -107,6 +108,7 @@ static void max17040_reset(struct i2c_client *client)
>  static int max17040_set_low_soc_threshold_alert(struct i2c_client *client,
>         u32 level)
>  {
> +       struct max17040_chip *chip = i2c_get_clientdata(client);
>         int ret;
>         u16 data;
>
> @@ -118,6 +120,7 @@ static int max17040_set_low_soc_threshold_alert(struct i2c_client *client,
>                 data &= MAX17040_ATHD_MASK;
>                 data |= level;
>                 max17040_write_reg(client, MAX17040_RCOMP, data);
> +               chip->alert_bit = 0;
>                 ret = 0;
>         } else {
>                 ret = -EINVAL;
> @@ -144,6 +147,11 @@ static void max17040_get_soc(struct i2c_client *client)
>         soc = max17040_read_reg(client, MAX17040_SOC);
>
>         chip->soc = (soc >> 8);
> +
> +       /* check SOC level to clear ALRT bit */
> +       if (chip->soc > chip->low_soc_alert_threshold && chip->alert_bit)
> +               max17040_set_low_soc_threshold_alert(client,
> +                       chip->low_soc_alert_threshold);
>  }
>
>  static void max17040_get_version(struct i2c_client *client)
> @@ -229,6 +237,9 @@ static irqreturn_t max17040_thread_handler(int id, void *dev)
>         /* send uevent */
>         power_supply_changed(chip->battery);
>
> +       /* ALRT bit is seted */

s/seted/set/

Best regards,
Krzysztof

> +       chip->alert_bit = 1;
> +
>         return IRQ_HANDLED;
>  }
>
> --
> 2.20.1
>

[PATCH v3 5/5] power: supply: max17040: Send uevent in SOC and status change

[Matheus Castello]

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 <matheus@castello.eng.br>
---
 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

Re: [PATCH v3 5/5] power: supply: max17040: Send uevent in SOC and status change

[Krzysztof Kozlowski]

On Mon, 27 May 2019 at 04:23, Matheus Castello <matheus@castello.eng.br> 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 <matheus@castello.eng.br>
> ---
>  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
>

[PATCH v4 0/4] power: supply: MAX17040: Add IRQ for low level and alert SOC changes

[Matheus Castello]

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

[PATCH v4 1/4] power: supply: max17040: Add IRQ handler for low SOC alert

[Matheus Castello]

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 <matheus@castello.eng.br>
---
 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

Re: [PATCH v4 1/4] power: supply: max17040: Add IRQ handler for low SOC alert

[Krzysztof Kozlowski]

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 <matheus@castello.eng.br>
> ---
>  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 <krzk@kernel.org>

Best regards,
Krzysztof

[PATCH v4 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold

[Matheus Castello]

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 <matheus@castello.eng.br>
---
 .../power/supply/max17040_battery.txt         | 33 +++++++++++++++++++
 1 file changed, 33 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..f2d0b22b5f79
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
@@ -0,0 +1,33 @@
+max17040_battery
+~~~~~~~~~~~~~~~~
+
+Required properties :
+ - compatible : "maxim,max17040" or "maxim,max77836-battery"
+ - reg: i2c slave address
+
+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.
+
+Optional properties support interrupt functionality for alert low state of
+charge level, present in some ICs in the same family, and should be used with
+compatible "maxim,max77836-battery".
+
+Example:
+
+	battery-fuel-gauge@36 {
+		compatible = "maxim,max77836-battery";
+		reg = <0x36>;
+		maxim,alert-low-soc-level = <10>;
+		interrupt-parent = <&gpio7>;
+		interrupts = <2 IRQ_TYPE_EDGE_FALLING>;
+		wakeup-source;
+	};
--
2.24.0.rc2

Re: [PATCH v4 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold

[Krzysztof Kozlowski]

On Thu, Oct 31, 2019 at 03:41:32PM -0300, Matheus Castello 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 <matheus@castello.eng.br>
> ---
>  .../power/supply/max17040_battery.txt         | 33 +++++++++++++++++++
>  1 file changed, 33 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH v4 2/4] dt-bindings: power: supply: Max17040: Add low level SOC alert threshold

[Rob Herring]

On Thu, Oct 31, 2019 at 03:41:32PM -0300, Matheus Castello wrote:
> For configure low level state of charge threshold alert signaled from
> max17040 we add "maxim,alert-low-soc-level" property.

This doesn't match the change. You're adding a whole new device binding.

> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> ---
>  .../power/supply/max17040_battery.txt         | 33 +++++++++++++++++++

Part of an MFD? The MFD binding should reference this file then.

>  1 file changed, 33 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..f2d0b22b5f79
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
> @@ -0,0 +1,33 @@
> +max17040_battery
> +~~~~~~~~~~~~~~~~
> +
> +Required properties :
> + - compatible : "maxim,max17040" or "maxim,max77836-battery"
> + - reg: i2c slave address
> +
> +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.

Seems like something that should be a common battery property.

> +- 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.
> +
> +Optional properties support interrupt functionality for alert low state of
> +charge level, present in some ICs in the same family, and should be used with
> +compatible "maxim,max77836-battery".
> +
> +Example:
> +
> +	battery-fuel-gauge@36 {
> +		compatible = "maxim,max77836-battery";
> +		reg = <0x36>;
> +		maxim,alert-low-soc-level = <10>;
> +		interrupt-parent = <&gpio7>;
> +		interrupts = <2 IRQ_TYPE_EDGE_FALLING>;
> +		wakeup-source;
> +	};
> --
> 2.24.0.rc2
> 

[PATCH v5 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes

[Matheus Castello]

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 v4:
(Suggested by Krzysztof Kozlowski)
- Fix code style and alignment issues
- Keep IRQF_TRIGGER_FALLING | IRQF_ONESHOT instead client->flags

(Suggested by Rob Herring)
- Add reference to the MFD description
- Fix the dt-bindings commit description

Matheus Castello (5):
  power: supply: max17040: Add IRQ handler for low SOC alert
  dt-bindings: power: supply: Max17040: Add DT bindings for max17040
    fuel gauge
  devicetree: mfd: max14577: Add reference to max14040_battery.txt
    descriptions
  power: supply: max17040: Config alert SOC low level threshold from FDT
  power: supply: max17040: Send uevent in SOC and status change

 .../devicetree/bindings/mfd/max14577.txt      |   2 +
 .../power/supply/max17040_battery.txt         |  33 ++++
 drivers/power/supply/max17040_battery.c       | 142 +++++++++++++++++-
 3 files changed, 172 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt

--
2.24.0.rc2

[PATCH v5 1/5] power: supply: max17040: Add IRQ handler for low SOC alert

[Matheus Castello]

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 <matheus@castello.eng.br>
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 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

[PATCH v5 2/5] dt-bindings: power: supply: Max17040: Add DT bindings for max17040 fuel gauge

[Matheus Castello]

Documentation of max17040 based fuel gauge characteristics.
For configure low level state of charge threshold alert signaled from
max17043/max17044 we add "maxim,alert-low-soc-level" property.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 .../power/supply/max17040_battery.txt         | 33 +++++++++++++++++++
 1 file changed, 33 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..f2d0b22b5f79
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
@@ -0,0 +1,33 @@
+max17040_battery
+~~~~~~~~~~~~~~~~
+
+Required properties :
+ - compatible : "maxim,max17040" or "maxim,max77836-battery"
+ - reg: i2c slave address
+
+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.
+
+Optional properties support interrupt functionality for alert low state of
+charge level, present in some ICs in the same family, and should be used with
+compatible "maxim,max77836-battery".
+
+Example:
+
+	battery-fuel-gauge@36 {
+		compatible = "maxim,max77836-battery";
+		reg = <0x36>;
+		maxim,alert-low-soc-level = <10>;
+		interrupt-parent = <&gpio7>;
+		interrupts = <2 IRQ_TYPE_EDGE_FALLING>;
+		wakeup-source;
+	};
--
2.24.0.rc2

[PATCH v5 3/5] devicetree: mfd: max14577: Add reference to max14040_battery.txt descriptions

[Matheus Castello]

max77836 MFD has a fuel gauge that has a low SOC alert feature that is
described in Documentation/devicetree/bindings/power/supply/max17040_battery.txt.
Adding the reference to de documentation here.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 Documentation/devicetree/bindings/mfd/max14577.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/max14577.txt b/Documentation/devicetree/bindings/mfd/max14577.txt
index fc6f0f4e8beb..04d790b1513c 100644
--- a/Documentation/devicetree/bindings/mfd/max14577.txt
+++ b/Documentation/devicetree/bindings/mfd/max14577.txt
@@ -5,6 +5,8 @@ Battery Charger and SFOUT LDO output for powering USB devices. It is
 interfaced to host controller using I2C.

 MAX77836 additionally contains PMIC (with two LDO regulators) and Fuel Gauge.
+For the description of Fuel Gauge low SOC alert interrupt see:
+Documentation/devicetree/bindings/power/supply/max17040_battery.txt


 Required properties:
--
2.24.0.rc2

[PATCH v5 4/5] power: supply: max17040: Config alert SOC low level threshold from FDT

[Matheus Castello]

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 <matheus@castello.eng.br>
---
 drivers/power/supply/max17040_battery.c | 96 +++++++++++++++++++++----
 1 file changed, 82 insertions(+), 14 deletions(-)

diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
index 75459f76d02c..c48a691cbd7b 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;
 };

 static int max17040_get_property(struct power_supply *psy,
@@ -99,6 +104,21 @@ static void max17040_reset(struct i2c_client *client)
 	max17040_write_reg(client, MAX17040_CMD, 0x0054);
 }

+static int max17040_set_low_soc_alert(struct i2c_client *client, u32 level)
+{
+	int ret;
+	u16 data;
+
+	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;
+	ret = max17040_write_reg(client, MAX17040_RCOMP, data);
+
+	return ret;
+}
+
 static void max17040_get_vcell(struct i2c_client *client)
 {
 	struct max17040_chip *chip = i2c_get_clientdata(client);
@@ -115,7 +135,6 @@ static void max17040_get_soc(struct i2c_client *client)
 	u16 soc;

 	soc = max17040_read_reg(client, MAX17040_SOC);
-
 	chip->soc = (soc >> 8);
 }

@@ -161,6 +180,24 @@ static void max17040_get_status(struct i2c_client *client)
 		chip->status = POWER_SUPPLY_STATUS_FULL;
 }

+static int max17040_get_of_data(struct max17040_chip *chip)
+{
+	struct device *dev = &chip->client->dev;
+	struct device_node *np = dev->of_node;
+	int ret = 0;
+
+	if (of_property_read_u32(np, "maxim,alert-low-soc-level",
+				 &chip->low_soc_alert)) {
+		chip->low_soc_alert = MAX17040_ATHD_DEFAULT_POWER_UP;
+	} else if (chip->low_soc_alert <= 0 ||
+			chip->low_soc_alert >= 33) {
+		/* low_soc_alert is not between 1% and 32% */
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
 static void max17040_check_changes(struct i2c_client *client)
 {
 	max17040_get_vcell(client);
@@ -192,9 +229,27 @@ static irqreturn_t max17040_thread_handler(int id, void *dev)
 	/* send uevent */
 	power_supply_changed(chip->battery);

+	/* reset alert bit */
+	max17040_set_low_soc_alert(client, chip->low_soc_alert);
+
 	return IRQ_HANDLED;
 }

+static int max17040_enable_alert_irq(struct max17040_chip *chip)
+{
+	struct i2c_client *client = chip->client;
+	unsigned int flags;
+	int 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,
+					chip->battery->desc->name, chip);
+
+	return ret;
+}
+
 static enum power_supply_property max17040_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_ONLINE,
@@ -216,6 +271,7 @@ static int max17040_probe(struct i2c_client *client,
 	struct i2c_adapter *adapter = client->adapter;
 	struct power_supply_config psy_cfg = {};
 	struct max17040_chip *chip;
+	int ret;

 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
 		return -EIO;
@@ -226,6 +282,12 @@ static int max17040_probe(struct i2c_client *client,

 	chip->client = client;
 	chip->pdata = client->dev.platform_data;
+	ret = max17040_get_of_data(chip);
+	if (ret) {
+		dev_err(&client->dev,
+			"failed: low SOC alert OF data out of bounds\n");
+		return ret;
+	}

 	i2c_set_clientdata(client, chip);
 	psy_cfg.drv_data = chip;
@@ -242,20 +304,26 @@ static int max17040_probe(struct i2c_client *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;
+		if (of_device_is_compatible(client->dev.of_node,
+					    "maxim,max77836-battery")) {
+			ret = max17040_set_low_soc_alert(client,
+							 chip->low_soc_alert);
+			if (ret) {
+				dev_err(&client->dev,
+					"Failed to set low SOC alert: err %d\n",
+					ret);
+				return ret;
+			}
+
+			ret = max17040_enable_alert_irq(chip);
+			if (ret) {
+				client->irq = 0;
+				dev_warn(&client->dev,
+					 "Failed to get IRQ err %d\n", ret);
+			}
+		} else {
 			dev_warn(&client->dev,
-				"Failed to get IRQ err %d\n", ret);
+				 "Device not compatible for IRQ");
 		}
 	}

--
2.24.0.rc2

Re: [PATCH v5 4/5] power: supply: max17040: Config alert SOC low level threshold from FDT

[Krzysztof Kozlowski]

On Tue, Nov 05, 2019 at 02:42:17AM -0300, Matheus Castello 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 <matheus@castello.eng.br>
> ---
>  drivers/power/supply/max17040_battery.c | 96 +++++++++++++++++++++----
>  1 file changed, 82 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
> index 75459f76d02c..c48a691cbd7b 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;
>  };
> 
>  static int max17040_get_property(struct power_supply *psy,
> @@ -99,6 +104,21 @@ static void max17040_reset(struct i2c_client *client)
>  	max17040_write_reg(client, MAX17040_CMD, 0x0054);
>  }
> 
> +static int max17040_set_low_soc_alert(struct i2c_client *client, u32 level)
> +{
> +	int ret;
> +	u16 data;
> +
> +	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;
> +	ret = max17040_write_reg(client, MAX17040_RCOMP, data);
> +
> +	return ret;
> +}
> +
>  static void max17040_get_vcell(struct i2c_client *client)
>  {
>  	struct max17040_chip *chip = i2c_get_clientdata(client);
> @@ -115,7 +135,6 @@ static void max17040_get_soc(struct i2c_client *client)
>  	u16 soc;
> 
>  	soc = max17040_read_reg(client, MAX17040_SOC);
> -
>  	chip->soc = (soc >> 8);
>  }
> 
> @@ -161,6 +180,24 @@ static void max17040_get_status(struct i2c_client *client)
>  		chip->status = POWER_SUPPLY_STATUS_FULL;
>  }
> 
> +static int max17040_get_of_data(struct max17040_chip *chip)
> +{
> +	struct device *dev = &chip->client->dev;
> +	struct device_node *np = dev->of_node;
> +	int ret = 0;
> +
> +	if (of_property_read_u32(np, "maxim,alert-low-soc-level",
> +				 &chip->low_soc_alert)) {
> +		chip->low_soc_alert = MAX17040_ATHD_DEFAULT_POWER_UP;
> +	} else if (chip->low_soc_alert <= 0 ||
> +			chip->low_soc_alert >= 33) {
> +		/* low_soc_alert is not between 1% and 32% */
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
>  static void max17040_check_changes(struct i2c_client *client)
>  {
>  	max17040_get_vcell(client);
> @@ -192,9 +229,27 @@ static irqreturn_t max17040_thread_handler(int id, void *dev)
>  	/* send uevent */
>  	power_supply_changed(chip->battery);
> 
> +	/* reset alert bit */
> +	max17040_set_low_soc_alert(client, chip->low_soc_alert);
> +
>  	return IRQ_HANDLED;
>  }
> 
> +static int max17040_enable_alert_irq(struct max17040_chip *chip)
> +{

It does not make really sense to move this code to separate function in
this patch. This should be done in patch 1/5. Otherwise you add a code
in patch 1 and later in patch 4 you immediately rearrange it. This
raises eybrows and gives a hint that patchset is not well structured.

> +	struct i2c_client *client = chip->client;
> +	unsigned int flags;
> +	int ret;
> +
> +	dev_info(&client->dev, "IRQ: enabled\n");

While at it, get rid of dev_info here. It does not bring any useful
information. All this is available in /proc/interrupts.

Best regards,
Krzysztof

> +	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);
> +
> +	return ret;
> +}
> +
>  static enum power_supply_property max17040_battery_props[] = {
>  	POWER_SUPPLY_PROP_STATUS,
>  	POWER_SUPPLY_PROP_ONLINE,
> @@ -216,6 +271,7 @@ static int max17040_probe(struct i2c_client *client,
>  	struct i2c_adapter *adapter = client->adapter;
>  	struct power_supply_config psy_cfg = {};
>  	struct max17040_chip *chip;
> +	int ret;
> 
>  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
>  		return -EIO;
> @@ -226,6 +282,12 @@ static int max17040_probe(struct i2c_client *client,
> 
>  	chip->client = client;
>  	chip->pdata = client->dev.platform_data;
> +	ret = max17040_get_of_data(chip);
> +	if (ret) {
> +		dev_err(&client->dev,
> +			"failed: low SOC alert OF data out of bounds\n");
> +		return ret;
> +	}
> 
>  	i2c_set_clientdata(client, chip);
>  	psy_cfg.drv_data = chip;
> @@ -242,20 +304,26 @@ static int max17040_probe(struct i2c_client *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;
> +		if (of_device_is_compatible(client->dev.of_node,
> +					    "maxim,max77836-battery")) {
> +			ret = max17040_set_low_soc_alert(client,
> +							 chip->low_soc_alert);
> +			if (ret) {
> +				dev_err(&client->dev,
> +					"Failed to set low SOC alert: err %d\n",
> +					ret);
> +				return ret;
> +			}
> +
> +			ret = max17040_enable_alert_irq(chip);
> +			if (ret) {
> +				client->irq = 0;
> +				dev_warn(&client->dev,
> +					 "Failed to get IRQ err %d\n", ret);
> +			}
> +		} else {
>  			dev_warn(&client->dev,
> -				"Failed to get IRQ err %d\n", ret);
> +				 "Device not compatible for IRQ");
>  		}
>  	}
> 
> --
> 2.24.0.rc2
> 

[PATCH v6 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes

[Matheus Castello]

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 v5:
(Suggested by Krzysztof Kozlowski)
- Rearrange code and add max17040_enable_alert_irq on patch 1/5
- Remove useless dev_info

Changes since v4:
(Suggested by Krzysztof Kozlowski)
- Fix code style and alignment issues
- Keep IRQF_TRIGGER_FALLING | IRQF_ONESHOT instead client->flags

(Suggested by Rob Herring)
- Add reference to the MFD description
- Fix the dt-bindings commit description

Matheus Castello (5):
  power: supply: max17040: Add IRQ handler for low SOC alert
  dt-bindings: power: supply: Max17040: Add DT bindings for max17040
    fuel gauge
  devicetree: mfd: max14577: Add reference to max14040_battery.txt
    descriptions
  power: supply: max17040: Config alert SOC low level threshold from FDT
  power: supply: max17040: Send uevent in SOC and status change

 .../devicetree/bindings/mfd/max14577.txt      |   2 +
 .../power/supply/max17040_battery.txt         |  33 ++++
 drivers/power/supply/max17040_battery.c       | 141 +++++++++++++++++-
 3 files changed, 171 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt

--
2.24.0.rc2

[PATCH v6 1/5] power: supply: max17040: Add IRQ handler for low SOC alert

[Matheus Castello]

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 <matheus@castello.eng.br>
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/power/supply/max17040_battery.c | 73 +++++++++++++++++++++++--
 1 file changed, 68 insertions(+), 5 deletions(-)

diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
index 62499018e68b..9909f8cd7b5d 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,54 @@ 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 int max17040_enable_alert_irq(struct max17040_chip *chip)
+{
+	struct i2c_client *client = chip->client;
+	unsigned int flags;
+	int ret;
+
+	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);
+
+	return ret;
+}
+
 static enum power_supply_property max17040_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_ONLINE,
@@ -220,6 +254,19 @@ static int max17040_probe(struct i2c_client *client,
 	max17040_reset(client);
 	max17040_get_version(client);

+	/* check interrupt */
+	if (client->irq) {
+		int ret;
+
+		ret = max17040_enable_alert_irq(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 +291,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 +309,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

[PATCH v6 2/5] dt-bindings: power: supply: Max17040: Add DT bindings for max17040 fuel gauge

[Matheus Castello]

Documentation of max17040 based fuel gauge characteristics.
For configure low level state of charge threshold alert signaled from
max17043/max17044 we add "maxim,alert-low-soc-level" property.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 .../power/supply/max17040_battery.txt         | 33 +++++++++++++++++++
 1 file changed, 33 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..f2d0b22b5f79
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
@@ -0,0 +1,33 @@
+max17040_battery
+~~~~~~~~~~~~~~~~
+
+Required properties :
+ - compatible : "maxim,max17040" or "maxim,max77836-battery"
+ - reg: i2c slave address
+
+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.
+
+Optional properties support interrupt functionality for alert low state of
+charge level, present in some ICs in the same family, and should be used with
+compatible "maxim,max77836-battery".
+
+Example:
+
+	battery-fuel-gauge@36 {
+		compatible = "maxim,max77836-battery";
+		reg = <0x36>;
+		maxim,alert-low-soc-level = <10>;
+		interrupt-parent = <&gpio7>;
+		interrupts = <2 IRQ_TYPE_EDGE_FALLING>;
+		wakeup-source;
+	};
--
2.24.0.rc2

Re: [PATCH v6 2/5] dt-bindings: power: supply: Max17040: Add DT bindings for max17040 fuel gauge

[Rob Herring]

On Thu,  7 Nov 2019 00:17:07 -0300, Matheus Castello wrote:
> Documentation of max17040 based fuel gauge characteristics.
> For configure low level state of charge threshold alert signaled from
> max17043/max17044 we add "maxim,alert-low-soc-level" property.
> 
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  .../power/supply/max17040_battery.txt         | 33 +++++++++++++++++++
>  1 file changed, 33 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/supply/max17040_battery.txt
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v6 3/5] devicetree: mfd: max14577: Add reference to max14040_battery.txt descriptions

[Matheus Castello]

max77836 MFD has a fuel gauge that has a low SOC alert feature that is
described in Documentation/devicetree/bindings/power/supply/max17040_battery.txt.
Adding the reference to de documentation here.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 Documentation/devicetree/bindings/mfd/max14577.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/max14577.txt b/Documentation/devicetree/bindings/mfd/max14577.txt
index fc6f0f4e8beb..04d790b1513c 100644
--- a/Documentation/devicetree/bindings/mfd/max14577.txt
+++ b/Documentation/devicetree/bindings/mfd/max14577.txt
@@ -5,6 +5,8 @@ Battery Charger and SFOUT LDO output for powering USB devices. It is
 interfaced to host controller using I2C.

 MAX77836 additionally contains PMIC (with two LDO regulators) and Fuel Gauge.
+For the description of Fuel Gauge low SOC alert interrupt see:
+Documentation/devicetree/bindings/power/supply/max17040_battery.txt


 Required properties:
--
2.24.0.rc2

Re: [PATCH v6 3/5] devicetree: mfd: max14577: Add reference to max14040_battery.txt descriptions

[Lee Jones]

On Thu, 07 Nov 2019, Matheus Castello wrote:

> max77836 MFD has a fuel gauge that has a low SOC alert feature that is
> described in Documentation/devicetree/bindings/power/supply/max17040_battery.txt.
> Adding the reference to de documentation here.
> 
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> ---
>  Documentation/devicetree/bindings/mfd/max14577.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/max14577.txt b/Documentation/devicetree/bindings/mfd/max14577.txt
> index fc6f0f4e8beb..04d790b1513c 100644
> --- a/Documentation/devicetree/bindings/mfd/max14577.txt
> +++ b/Documentation/devicetree/bindings/mfd/max14577.txt
> @@ -5,6 +5,8 @@ Battery Charger and SFOUT LDO output for powering USB devices. It is
>  interfaced to host controller using I2C.
> 
>  MAX77836 additionally contains PMIC (with two LDO regulators) and Fuel Gauge.
> +For the description of Fuel Gauge low SOC alert interrupt see:
> +Documentation/devicetree/bindings/power/supply/max17040_battery.txt

I would prefer the use of relative paths in documentation.

Once fixed, please apply my:

Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v6 3/5] devicetree: mfd: max14577: Add reference to max14040_battery.txt descriptions

[Rob Herring]

On Thu,  7 Nov 2019 00:17:08 -0300, Matheus Castello wrote:
> max77836 MFD has a fuel gauge that has a low SOC alert feature that is
> described in Documentation/devicetree/bindings/power/supply/max17040_battery.txt.
> Adding the reference to de documentation here.
> 
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> ---
>  Documentation/devicetree/bindings/mfd/max14577.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

[PATCH v6 4/5] power: supply: max17040: Config alert SOC low level threshold from FDT

[Matheus Castello]

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 <matheus@castello.eng.br>
---
 drivers/power/supply/max17040_battery.c | 75 ++++++++++++++++++++++---
 1 file changed, 67 insertions(+), 8 deletions(-)

diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
index 9909f8cd7b5d..3fc9e1c7b257 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;
 };

 static int max17040_get_property(struct power_supply *psy,
@@ -99,6 +104,21 @@ static void max17040_reset(struct i2c_client *client)
 	max17040_write_reg(client, MAX17040_CMD, 0x0054);
 }

+static int max17040_set_low_soc_alert(struct i2c_client *client, u32 level)
+{
+	int ret;
+	u16 data;
+
+	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;
+	ret = max17040_write_reg(client, MAX17040_RCOMP, data);
+
+	return ret;
+}
+
 static void max17040_get_vcell(struct i2c_client *client)
 {
 	struct max17040_chip *chip = i2c_get_clientdata(client);
@@ -115,7 +135,6 @@ static void max17040_get_soc(struct i2c_client *client)
 	u16 soc;

 	soc = max17040_read_reg(client, MAX17040_SOC);
-
 	chip->soc = (soc >> 8);
 }

@@ -161,6 +180,24 @@ static void max17040_get_status(struct i2c_client *client)
 		chip->status = POWER_SUPPLY_STATUS_FULL;
 }

+static int max17040_get_of_data(struct max17040_chip *chip)
+{
+	struct device *dev = &chip->client->dev;
+	struct device_node *np = dev->of_node;
+	int ret = 0;
+
+	if (of_property_read_u32(np, "maxim,alert-low-soc-level",
+				 &chip->low_soc_alert)) {
+		chip->low_soc_alert = MAX17040_ATHD_DEFAULT_POWER_UP;
+	} else if (chip->low_soc_alert <= 0 ||
+			chip->low_soc_alert >= 33) {
+		/* low_soc_alert is not between 1% and 32% */
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
 static void max17040_check_changes(struct i2c_client *client)
 {
 	max17040_get_vcell(client);
@@ -192,6 +229,9 @@ static irqreturn_t max17040_thread_handler(int id, void *dev)
 	/* send uevent */
 	power_supply_changed(chip->battery);

+	/* reset alert bit */
+	max17040_set_low_soc_alert(client, chip->low_soc_alert);
+
 	return IRQ_HANDLED;
 }

@@ -230,6 +270,7 @@ static int max17040_probe(struct i2c_client *client,
 	struct i2c_adapter *adapter = client->adapter;
 	struct power_supply_config psy_cfg = {};
 	struct max17040_chip *chip;
+	int ret;

 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
 		return -EIO;
@@ -240,6 +281,12 @@ static int max17040_probe(struct i2c_client *client,

 	chip->client = client;
 	chip->pdata = client->dev.platform_data;
+	ret = max17040_get_of_data(chip);
+	if (ret) {
+		dev_err(&client->dev,
+			"failed: low SOC alert OF data out of bounds\n");
+		return ret;
+	}

 	i2c_set_clientdata(client, chip);
 	psy_cfg.drv_data = chip;
@@ -256,14 +303,26 @@ static int max17040_probe(struct i2c_client *client,

 	/* check interrupt */
 	if (client->irq) {
-		int ret;
-
-		ret = max17040_enable_alert_irq(chip);
-
-		if (ret) {
-			client->irq = 0;
+		if (of_device_is_compatible(client->dev.of_node,
+					    "maxim,max77836-battery")) {
+			ret = max17040_set_low_soc_alert(client,
+							 chip->low_soc_alert);
+			if (ret) {
+				dev_err(&client->dev,
+					"Failed to set low SOC alert: err %d\n",
+					ret);
+				return ret;
+			}
+
+			ret = max17040_enable_alert_irq(chip);
+			if (ret) {
+				client->irq = 0;
+				dev_warn(&client->dev,
+					 "Failed to get IRQ err %d\n", ret);
+			}
+		} else {
 			dev_warn(&client->dev,
-				 "Failed to get IRQ err %d\n", ret);
+				 "Device not compatible for IRQ");
 		}
 	}

--
2.24.0.rc2

[PATCH v6 5/5] power: supply: max17040: Send uevent in SOC and status change

[Matheus Castello]

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 <matheus@castello.eng.br>
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/power/supply/max17040_battery.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
index 3fc9e1c7b257..1f5afabdbabc 100644
--- a/drivers/power/supply/max17040_battery.c
+++ b/drivers/power/supply/max17040_battery.c
@@ -209,10 +209,19 @@ static void max17040_check_changes(struct i2c_client *client)
 static void max17040_work(struct work_struct *work)
 {
 	struct max17040_chip *chip;
+	int last_soc, last_status;

 	chip = container_of(work, struct max17040_chip, work.work);
+
+	/* store SOC and status to check changes */
+	last_soc = chip->soc;
+	last_status = chip->status;
 	max17040_check_changes(chip->client);

+	/* check changes and send uevent */
+	if (last_soc != chip->soc || last_status != chip->status)
+		power_supply_changed(chip->battery);
+
 	queue_delayed_work(system_power_efficient_wq, &chip->work,
 			   MAX17040_DELAY);
 }
--
2.24.0.rc2

Re: [PATCH v6 0/5] power: supply: MAX17040: Add IRQ for low level and alert SOC changes

[Lee Jones]

On Thu, 07 Nov 2019, 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 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 v5:
> (Suggested by Krzysztof Kozlowski)
> - Rearrange code and add max17040_enable_alert_irq on patch 1/5
> - Remove useless dev_info

Just something to bear in mind in the future:

When submitting subsequent versions, could you please do so as
separate sets? Attaching them to previous submissions gets confusing
real quick.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v5 5/5] power: supply: max17040: Send uevent in SOC and status change

[Matheus Castello]

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 <matheus@castello.eng.br>
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/power/supply/max17040_battery.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
index c48a691cbd7b..d7405650cc38 100644
--- a/drivers/power/supply/max17040_battery.c
+++ b/drivers/power/supply/max17040_battery.c
@@ -209,10 +209,19 @@ static void max17040_check_changes(struct i2c_client *client)
 static void max17040_work(struct work_struct *work)
 {
 	struct max17040_chip *chip;
+	int last_soc, last_status;

 	chip = container_of(work, struct max17040_chip, work.work);
+
+	/* store SOC and status to check changes */
+	last_soc = chip->soc;
+	last_status = chip->status;
 	max17040_check_changes(chip->client);

+	/* check changes and send uevent */
+	if (last_soc != chip->soc || last_status != chip->status)
+		power_supply_changed(chip->battery);
+
 	queue_delayed_work(system_power_efficient_wq, &chip->work,
 			   MAX17040_DELAY);
 }
--
2.24.0.rc2

[PATCH v4 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT

[Matheus Castello]

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 <matheus@castello.eng.br>
---
 drivers/power/supply/max17040_battery.c | 88 +++++++++++++++++++++----
 1 file changed, 74 insertions(+), 14 deletions(-)

diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
index 75459f76d02c..802575342c72 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,22 @@ 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;
+
+	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;
+	ret = max17040_write_reg(client, MAX17040_RCOMP, data);
+
+	return ret;
+}
+
 static void max17040_get_vcell(struct i2c_client *client)
 {
 	struct max17040_chip *chip = i2c_get_clientdata(client);
@@ -115,7 +136,6 @@ static void max17040_get_soc(struct i2c_client *client)
 	u16 soc;

 	soc = max17040_read_reg(client, MAX17040_SOC);
-
 	chip->soc = (soc >> 8);
 }

@@ -161,6 +181,24 @@ static void max17040_get_status(struct i2c_client *client)
 		chip->status = POWER_SUPPLY_STATUS_FULL;
 }

+static int max17040_get_of_data(struct max17040_chip *chip)
+{
+	struct device *dev = &chip->client->dev;
+	struct device_node *np = dev->of_node;
+	int ret = 0;
+
+	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;
+	/* check if low_soc_alert_threshold is between 1% and 32% */
+	} else if (chip->low_soc_alert_threshold <= 0 ||
+			chip->low_soc_alert_threshold >= 33){
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
 static void max17040_check_changes(struct i2c_client *client)
 {
 	max17040_get_vcell(client);
@@ -192,6 +230,10 @@ static irqreturn_t max17040_thread_handler(int id, void *dev)
 	/* send uevent */
 	power_supply_changed(chip->battery);

+	/* reset alert bit */
+	max17040_set_low_soc_threshold_alert(client,
+		chip->low_soc_alert_threshold);
+
 	return IRQ_HANDLED;
 }

@@ -216,6 +258,7 @@ static int max17040_probe(struct i2c_client *client,
 	struct i2c_adapter *adapter = client->adapter;
 	struct power_supply_config psy_cfg = {};
 	struct max17040_chip *chip;
+	int ret;

 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
 		return -EIO;
@@ -226,6 +269,12 @@ static int max17040_probe(struct i2c_client *client,

 	chip->client = client;
 	chip->pdata = client->dev.platform_data;
+	ret = max17040_get_of_data(chip);
+	if (ret) {
+		dev_err(&client->dev,
+			"failed: low SOC alert OF data out of bounds\n");
+		return ret;
+	}

 	i2c_set_clientdata(client, chip);
 	psy_cfg.drv_data = chip;
@@ -242,20 +291,31 @@ static int max17040_probe(struct i2c_client *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;
+		if (of_device_is_compatible(client->dev.of_node,
+			"maxim,max77836-battery")) {
+			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");
+			ret = devm_request_threaded_irq(&client->dev,
+				client->irq, NULL, max17040_thread_handler,
+				(client->flags | IRQF_ONESHOT),
+				chip->battery->desc->name, chip);
+
+			if (ret) {
+				client->irq = 0;
+				dev_warn(&client->dev,
+					"Failed to get IRQ err %d\n", ret);
+			}
+		} else {
 			dev_warn(&client->dev,
-				"Failed to get IRQ err %d\n", ret);
+				"Device not compatible for IRQ");
 		}
 	}

--
2.24.0.rc2

Re: [PATCH v4 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT

[Krzysztof Kozlowski]

On Thu, Oct 31, 2019 at 03:41:33PM -0300, Matheus Castello 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 <matheus@castello.eng.br>
> ---
>  drivers/power/supply/max17040_battery.c | 88 +++++++++++++++++++++----
>  1 file changed, 74 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
> index 75459f76d02c..802575342c72 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,22 @@ 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;
> +
> +	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;
> +	ret = max17040_write_reg(client, MAX17040_RCOMP, data);
> +
> +	return ret;
> +}
> +
>  static void max17040_get_vcell(struct i2c_client *client)
>  {
>  	struct max17040_chip *chip = i2c_get_clientdata(client);
> @@ -115,7 +136,6 @@ static void max17040_get_soc(struct i2c_client *client)
>  	u16 soc;
> 
>  	soc = max17040_read_reg(client, MAX17040_SOC);
> -
>  	chip->soc = (soc >> 8);
>  }
> 
> @@ -161,6 +181,24 @@ static void max17040_get_status(struct i2c_client *client)
>  		chip->status = POWER_SUPPLY_STATUS_FULL;
>  }
> 
> +static int max17040_get_of_data(struct max17040_chip *chip)
> +{
> +	struct device *dev = &chip->client->dev;
> +	struct device_node *np = dev->of_node;
> +	int ret = 0;
> +
> +	if (of_property_read_u32(np, "maxim,alert-low-soc-level",
> +			&chip->low_soc_alert_threshold)) {

Please align the line break with line above. checkpatch --strict might
give you hints about this.

> +		chip->low_soc_alert_threshold = MAX17040_ATHD_DEFAULT_POWER_UP;
> +	/* check if low_soc_alert_threshold is between 1% and 32% */

The comment looks misleading here, like it belongs to previous block.
Maybe put it inside else if {} block?

> +	} else if (chip->low_soc_alert_threshold <= 0 ||
> +			chip->low_soc_alert_threshold >= 33){

Missing space before {.

> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
>  static void max17040_check_changes(struct i2c_client *client)
>  {
>  	max17040_get_vcell(client);
> @@ -192,6 +230,10 @@ static irqreturn_t max17040_thread_handler(int id, void *dev)
>  	/* send uevent */
>  	power_supply_changed(chip->battery);
> 
> +	/* reset alert bit */
> +	max17040_set_low_soc_threshold_alert(client,
> +		chip->low_soc_alert_threshold);

Unless the continuation exceeds 80 character limit, please align it with
previous line.

> +
>  	return IRQ_HANDLED;
>  }
> 
> @@ -216,6 +258,7 @@ static int max17040_probe(struct i2c_client *client,
>  	struct i2c_adapter *adapter = client->adapter;
>  	struct power_supply_config psy_cfg = {};
>  	struct max17040_chip *chip;
> +	int ret;
> 
>  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
>  		return -EIO;
> @@ -226,6 +269,12 @@ static int max17040_probe(struct i2c_client *client,
> 
>  	chip->client = client;
>  	chip->pdata = client->dev.platform_data;
> +	ret = max17040_get_of_data(chip);
> +	if (ret) {
> +		dev_err(&client->dev,
> +			"failed: low SOC alert OF data out of bounds\n");
> +		return ret;
> +	}
> 
>  	i2c_set_clientdata(client, chip);
>  	psy_cfg.drv_data = chip;
> @@ -242,20 +291,31 @@ static int max17040_probe(struct i2c_client *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;
> +		if (of_device_is_compatible(client->dev.of_node,
> +			"maxim,max77836-battery")) {

Alignment.

> +			ret = max17040_set_low_soc_threshold_alert(client,
> +				chip->low_soc_alert_threshold);

Ditto.

> +			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");
> +			ret = devm_request_threaded_irq(&client->dev,
> +				client->irq, NULL, max17040_thread_handler,
> +				(client->flags | IRQF_ONESHOT),

This looks unrelated. Befor ethis were IRQF_TRIGGER_FALLING |
IRQF_ONESHOT, now you use client->flags. There is no reason why this
commit should change it.

> +				chip->battery->desc->name, chip);

This breaks alignment which was here before.

Best regards,
Krzysztof


> +
> +			if (ret) {
> +				client->irq = 0;
> +				dev_warn(&client->dev,
> +					"Failed to get IRQ err %d\n", ret);
> +			}
> +		} else {
>  			dev_warn(&client->dev,
> -				"Failed to get IRQ err %d\n", ret);
> +				"Device not compatible for IRQ");
>  		}
>  	}
> 
> --
> 2.24.0.rc2
> 

Re: [PATCH v4 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT

[Matheus Castello]

Em 11/1/19 12:27 PM, Krzysztof Kozlowski escreveu:
> On Thu, Oct 31, 2019 at 03:41:33PM -0300, Matheus Castello 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 <matheus@castello.eng.br>
>> ---
>>   drivers/power/supply/max17040_battery.c | 88 +++++++++++++++++++++----
>>   1 file changed, 74 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
>> index 75459f76d02c..802575342c72 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,22 @@ 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;
>> +
>> +	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;
>> +	ret = max17040_write_reg(client, MAX17040_RCOMP, data);
>> +
>> +	return ret;
>> +}
>> +
>>   static void max17040_get_vcell(struct i2c_client *client)
>>   {
>>   	struct max17040_chip *chip = i2c_get_clientdata(client);
>> @@ -115,7 +136,6 @@ static void max17040_get_soc(struct i2c_client *client)
>>   	u16 soc;
>>
>>   	soc = max17040_read_reg(client, MAX17040_SOC);
>> -
>>   	chip->soc = (soc >> 8);
>>   }
>>
>> @@ -161,6 +181,24 @@ static void max17040_get_status(struct i2c_client *client)
>>   		chip->status = POWER_SUPPLY_STATUS_FULL;
>>   }
>>
>> +static int max17040_get_of_data(struct max17040_chip *chip)
>> +{
>> +	struct device *dev = &chip->client->dev;
>> +	struct device_node *np = dev->of_node;
>> +	int ret = 0;
>> +
>> +	if (of_property_read_u32(np, "maxim,alert-low-soc-level",
>> +			&chip->low_soc_alert_threshold)) {
> 
> Please align the line break with line above. checkpatch --strict might
> give you hints about this.
> >> +		chip->low_soc_alert_threshold = MAX17040_ATHD_DEFAULT_POWER_UP;
>> +	/* check if low_soc_alert_threshold is between 1% and 32% */
> 
> The comment looks misleading here, like it belongs to previous block.
> Maybe put it inside else if {} block?
> 
>> +	} else if (chip->low_soc_alert_threshold <= 0 ||
>> +			chip->low_soc_alert_threshold >= 33){
> 
> Missing space before {.
> 
>> +		ret = -EINVAL;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>   static void max17040_check_changes(struct i2c_client *client)
>>   {
>>   	max17040_get_vcell(client);
>> @@ -192,6 +230,10 @@ static irqreturn_t max17040_thread_handler(int id, void *dev)
>>   	/* send uevent */
>>   	power_supply_changed(chip->battery);
>>
>> +	/* reset alert bit */
>> +	max17040_set_low_soc_threshold_alert(client,
>> +		chip->low_soc_alert_threshold);
> 
> Unless the continuation exceeds 80 character limit, please align it with
> previous line.
> 
>> +
>>   	return IRQ_HANDLED;
>>   }
>>
>> @@ -216,6 +258,7 @@ static int max17040_probe(struct i2c_client *client,
>>   	struct i2c_adapter *adapter = client->adapter;
>>   	struct power_supply_config psy_cfg = {};
>>   	struct max17040_chip *chip;
>> +	int ret;
>>
>>   	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
>>   		return -EIO;
>> @@ -226,6 +269,12 @@ static int max17040_probe(struct i2c_client *client,
>>
>>   	chip->client = client;
>>   	chip->pdata = client->dev.platform_data;
>> +	ret = max17040_get_of_data(chip);
>> +	if (ret) {
>> +		dev_err(&client->dev,
>> +			"failed: low SOC alert OF data out of bounds\n");
>> +		return ret;
>> +	}
>>
>>   	i2c_set_clientdata(client, chip);
>>   	psy_cfg.drv_data = chip;
>> @@ -242,20 +291,31 @@ static int max17040_probe(struct i2c_client *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;
>> +		if (of_device_is_compatible(client->dev.of_node,
>> +			"maxim,max77836-battery")) {
> 
> Alignment.
> 
>> +			ret = max17040_set_low_soc_threshold_alert(client,
>> +				chip->low_soc_alert_threshold);
> 
> Ditto.
> 
>> +			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");
>> +			ret = devm_request_threaded_irq(&client->dev,
>> +				client->irq, NULL, max17040_thread_handler,
>> +				(client->flags | IRQF_ONESHOT),
> 
> This looks unrelated. Befor ethis were IRQF_TRIGGER_FALLING |
> IRQF_ONESHOT, now you use client->flags. There is no reason why this
> commit should change >

I am using client->flags here to not overwrite the flag passed in device 
tree. Let me know what you think about it: if I should leave it as in 
the previous commit, or should I modify the previous commit too.

>> +				chip->battery->desc->name, chip);
> 
> This breaks alignment which was here before.
> 
> Best regards,
> Krzysztof
> 
> 

Thanks for the review i will work on it.

>> +
>> +			if (ret) {
>> +				client->irq = 0;
>> +				dev_warn(&client->dev,
>> +					"Failed to get IRQ err %d\n", ret);
>> +			}
>> +		} else {
>>   			dev_warn(&client->dev,
>> -				"Failed to get IRQ err %d\n", ret);
>> +				"Device not compatible for IRQ");
>>   		}
>>   	}
>>
>> --
>> 2.24.0.rc2
>>

Re: [PATCH v4 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT

[Matheus Castello]

Em 11/1/19 1:52 PM, Matheus Castello escreveu:
> 
> 
> Em 11/1/19 12:27 PM, Krzysztof Kozlowski escreveu:
>> On Thu, Oct 31, 2019 at 03:41:33PM -0300, Matheus Castello 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 <matheus@castello.eng.br>
>>> ---
>>>   drivers/power/supply/max17040_battery.c | 88 +++++++++++++++++++++----
>>>   1 file changed, 74 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/power/supply/max17040_battery.c 
>>> b/drivers/power/supply/max17040_battery.c
>>> index 75459f76d02c..802575342c72 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,22 @@ 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;
>>> +
>>> +    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;
>>> +    ret = max17040_write_reg(client, MAX17040_RCOMP, data);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>>   static void max17040_get_vcell(struct i2c_client *client)
>>>   {
>>>       struct max17040_chip *chip = i2c_get_clientdata(client);
>>> @@ -115,7 +136,6 @@ static void max17040_get_soc(struct i2c_client 
>>> *client)
>>>       u16 soc;
>>>
>>>       soc = max17040_read_reg(client, MAX17040_SOC);
>>> -
>>>       chip->soc = (soc >> 8);
>>>   }
>>>
>>> @@ -161,6 +181,24 @@ static void max17040_get_status(struct 
>>> i2c_client *client)
>>>           chip->status = POWER_SUPPLY_STATUS_FULL;
>>>   }
>>>
>>> +static int max17040_get_of_data(struct max17040_chip *chip)
>>> +{
>>> +    struct device *dev = &chip->client->dev;
>>> +    struct device_node *np = dev->of_node;
>>> +    int ret = 0;
>>> +
>>> +    if (of_property_read_u32(np, "maxim,alert-low-soc-level",
>>> +            &chip->low_soc_alert_threshold)) {
>>
>> Please align the line break with line above. checkpatch --strict might
>> give you hints about this.
>> >> +        chip->low_soc_alert_threshold = 
>> MAX17040_ATHD_DEFAULT_POWER_UP;
>>> +    /* check if low_soc_alert_threshold is between 1% and 32% */
>>
>> The comment looks misleading here, like it belongs to previous block.
>> Maybe put it inside else if {} block?
>>
>>> +    } else if (chip->low_soc_alert_threshold <= 0 ||
>>> +            chip->low_soc_alert_threshold >= 33){
>>
>> Missing space before {.
>>
>>> +        ret = -EINVAL;
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
>>>   static void max17040_check_changes(struct i2c_client *client)
>>>   {
>>>       max17040_get_vcell(client);
>>> @@ -192,6 +230,10 @@ static irqreturn_t max17040_thread_handler(int 
>>> id, void *dev)
>>>       /* send uevent */
>>>       power_supply_changed(chip->battery);
>>>
>>> +    /* reset alert bit */
>>> +    max17040_set_low_soc_threshold_alert(client,
>>> +        chip->low_soc_alert_threshold);
>>
>> Unless the continuation exceeds 80 character limit, please align it with
>> previous line.
>>
>>> +
>>>       return IRQ_HANDLED;
>>>   }
>>>
>>> @@ -216,6 +258,7 @@ static int max17040_probe(struct i2c_client *client,
>>>       struct i2c_adapter *adapter = client->adapter;
>>>       struct power_supply_config psy_cfg = {};
>>>       struct max17040_chip *chip;
>>> +    int ret;
>>>
>>>       if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
>>>           return -EIO;
>>> @@ -226,6 +269,12 @@ static int max17040_probe(struct i2c_client 
>>> *client,
>>>
>>>       chip->client = client;
>>>       chip->pdata = client->dev.platform_data;
>>> +    ret = max17040_get_of_data(chip);
>>> +    if (ret) {
>>> +        dev_err(&client->dev,
>>> +            "failed: low SOC alert OF data out of bounds\n");
>>> +        return ret;
>>> +    }
>>>
>>>       i2c_set_clientdata(client, chip);
>>>       psy_cfg.drv_data = chip;
>>> @@ -242,20 +291,31 @@ static int max17040_probe(struct i2c_client 
>>> *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;
>>> +        if (of_device_is_compatible(client->dev.of_node,
>>> +            "maxim,max77836-battery")) {
>>
>> Alignment.
>>
>>> +            ret = max17040_set_low_soc_threshold_alert(client,
>>> +                chip->low_soc_alert_threshold);
>>
>> Ditto.
>>

I am working to fix the alignments issues using the checkpath strict and 
I have a doubt. Here for example if I fix the check "Alignment should 
match open parenthesis" it will pass the 80 characters limit and will 
show me a warning.

>>> +            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");
>>> +            ret = devm_request_threaded_irq(&client->dev,
>>> +                client->irq, NULL, max17040_thread_handler,
>>> +                (client->flags | IRQF_ONESHOT),
>>
>> This looks unrelated. Befor ethis were IRQF_TRIGGER_FALLING |
>> IRQF_ONESHOT, now you use client->flags. There is no reason why this
>> commit should change >
> 
> I am using client->flags here to not overwrite the flag passed in device 
> tree. Let me know what you think about it: if I should leave it as in 
> the previous commit, or should I modify the previous commit too.
> 
>>> +                chip->battery->desc->name, chip);
>>
>> This breaks alignment which was here before.
>>

The same here.
How to proceed in this case?

Best Regards,
Matheus Castello

>> Best regards,
>> Krzysztof
>>
>>
> 
> Thanks for the review i will work on it.
> 
>>> +
>>> +            if (ret) {
>>> +                client->irq = 0;
>>> +                dev_warn(&client->dev,
>>> +                    "Failed to get IRQ err %d\n", ret);
>>> +            }
>>> +        } else {
>>>               dev_warn(&client->dev,
>>> -                "Failed to get IRQ err %d\n", ret);
>>> +                "Device not compatible for IRQ");
>>>           }
>>>       }
>>>
>>> -- 
>>> 2.24.0.rc2
>>>

Re: [PATCH v4 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT

[Krzysztof Kozlowski]

On Sat, Nov 02, 2019 at 03:12:44PM -0300, Matheus Castello wrote:
> 
> 
> Em 11/1/19 1:52 PM, Matheus Castello escreveu:
> > 
> > 
> > Em 11/1/19 12:27 PM, Krzysztof Kozlowski escreveu:
> > > On Thu, Oct 31, 2019 at 03:41:33PM -0300, Matheus Castello 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 <matheus@castello.eng.br>
> > > > ---
> > > >   drivers/power/supply/max17040_battery.c | 88 +++++++++++++++++++++----
> > > >   1 file changed, 74 insertions(+), 14 deletions(-)
> > > > 
> > > > diff --git a/drivers/power/supply/max17040_battery.c
> > > > b/drivers/power/supply/max17040_battery.c
> > > > index 75459f76d02c..802575342c72 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,22 @@ 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;
> > > > +
> > > > +    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;
> > > > +    ret = max17040_write_reg(client, MAX17040_RCOMP, data);
> > > > +
> > > > +    return ret;
> > > > +}
> > > > +
> > > >   static void max17040_get_vcell(struct i2c_client *client)
> > > >   {
> > > >       struct max17040_chip *chip = i2c_get_clientdata(client);
> > > > @@ -115,7 +136,6 @@ static void max17040_get_soc(struct
> > > > i2c_client *client)
> > > >       u16 soc;
> > > > 
> > > >       soc = max17040_read_reg(client, MAX17040_SOC);
> > > > -
> > > >       chip->soc = (soc >> 8);
> > > >   }
> > > > 
> > > > @@ -161,6 +181,24 @@ static void max17040_get_status(struct
> > > > i2c_client *client)
> > > >           chip->status = POWER_SUPPLY_STATUS_FULL;
> > > >   }
> > > > 
> > > > +static int max17040_get_of_data(struct max17040_chip *chip)
> > > > +{
> > > > +    struct device *dev = &chip->client->dev;
> > > > +    struct device_node *np = dev->of_node;
> > > > +    int ret = 0;
> > > > +
> > > > +    if (of_property_read_u32(np, "maxim,alert-low-soc-level",
> > > > +            &chip->low_soc_alert_threshold)) {
> > > 
> > > Please align the line break with line above. checkpatch --strict might
> > > give you hints about this.
> > > >> +        chip->low_soc_alert_threshold =
> > > MAX17040_ATHD_DEFAULT_POWER_UP;
> > > > +    /* check if low_soc_alert_threshold is between 1% and 32% */
> > > 
> > > The comment looks misleading here, like it belongs to previous block.
> > > Maybe put it inside else if {} block?
> > > 
> > > > +    } else if (chip->low_soc_alert_threshold <= 0 ||
> > > > +            chip->low_soc_alert_threshold >= 33){
> > > 
> > > Missing space before {.
> > > 
> > > > +        ret = -EINVAL;
> > > > +    }
> > > > +
> > > > +    return ret;
> > > > +}
> > > > +
> > > >   static void max17040_check_changes(struct i2c_client *client)
> > > >   {
> > > >       max17040_get_vcell(client);
> > > > @@ -192,6 +230,10 @@ static irqreturn_t
> > > > max17040_thread_handler(int id, void *dev)
> > > >       /* send uevent */
> > > >       power_supply_changed(chip->battery);
> > > > 
> > > > +    /* reset alert bit */
> > > > +    max17040_set_low_soc_threshold_alert(client,
> > > > +        chip->low_soc_alert_threshold);
> > > 
> > > Unless the continuation exceeds 80 character limit, please align it with
> > > previous line.
> > > 
> > > > +
> > > >       return IRQ_HANDLED;
> > > >   }
> > > > 
> > > > @@ -216,6 +258,7 @@ static int max17040_probe(struct i2c_client *client,
> > > >       struct i2c_adapter *adapter = client->adapter;
> > > >       struct power_supply_config psy_cfg = {};
> > > >       struct max17040_chip *chip;
> > > > +    int ret;
> > > > 
> > > >       if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
> > > >           return -EIO;
> > > > @@ -226,6 +269,12 @@ static int max17040_probe(struct i2c_client
> > > > *client,
> > > > 
> > > >       chip->client = client;
> > > >       chip->pdata = client->dev.platform_data;
> > > > +    ret = max17040_get_of_data(chip);
> > > > +    if (ret) {
> > > > +        dev_err(&client->dev,
> > > > +            "failed: low SOC alert OF data out of bounds\n");
> > > > +        return ret;
> > > > +    }
> > > > 
> > > >       i2c_set_clientdata(client, chip);
> > > >       psy_cfg.drv_data = chip;
> > > > @@ -242,20 +291,31 @@ static int max17040_probe(struct
> > > > i2c_client *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;
> > > > +        if (of_device_is_compatible(client->dev.of_node,
> > > > +            "maxim,max77836-battery")) {
> > > 
> > > Alignment.
> > > 
> > > > +            ret = max17040_set_low_soc_threshold_alert(client,
> > > > +                chip->low_soc_alert_threshold);
> > > 
> > > Ditto.
> > > 
> 
> I am working to fix the alignments issues using the checkpath strict and I
> have a doubt. Here for example if I fix the check "Alignment should match
> open parenthesis" it will pass the 80 characters limit and will show me a
> warning.

Indeed which is a hint that the code readabiltiy is affected by long
function name + two indentations + long variable name. You can split it
to different function (covering the IRQ case) which will reduce one
indentation.

Alternatively do not align it but add few more tabs before chip->:
		ret = max17040_set_low_soc_threshold_alert(client,
						chip->low_soc_alert_threshold);
Renaming low_soc_alert_threshold to something shorter can help as well
(soc_alert seems enough descriptive).


Best regards,
Krzysztof

Re: [PATCH v4 3/4] power: supply: max17040: Config alert SOC low level threshold from FDT

[Krzysztof Kozlowski]

On Fri, Nov 01, 2019 at 01:52:13PM -0300, Matheus Castello wrote:
> 
> 
> Em 11/1/19 12:27 PM, Krzysztof Kozlowski escreveu:
> > On Thu, Oct 31, 2019 at 03:41:33PM -0300, Matheus Castello 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 <matheus@castello.eng.br>
> > > ---
> > >   drivers/power/supply/max17040_battery.c | 88 +++++++++++++++++++++----
> > >   1 file changed, 74 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
> > > index 75459f76d02c..802575342c72 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,22 @@ 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;
> > > +
> > > +	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;
> > > +	ret = max17040_write_reg(client, MAX17040_RCOMP, data);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >   static void max17040_get_vcell(struct i2c_client *client)
> > >   {
> > >   	struct max17040_chip *chip = i2c_get_clientdata(client);
> > > @@ -115,7 +136,6 @@ static void max17040_get_soc(struct i2c_client *client)
> > >   	u16 soc;
> > > 
> > >   	soc = max17040_read_reg(client, MAX17040_SOC);
> > > -
> > >   	chip->soc = (soc >> 8);
> > >   }
> > > 
> > > @@ -161,6 +181,24 @@ static void max17040_get_status(struct i2c_client *client)
> > >   		chip->status = POWER_SUPPLY_STATUS_FULL;
> > >   }
> > > 
> > > +static int max17040_get_of_data(struct max17040_chip *chip)
> > > +{
> > > +	struct device *dev = &chip->client->dev;
> > > +	struct device_node *np = dev->of_node;
> > > +	int ret = 0;
> > > +
> > > +	if (of_property_read_u32(np, "maxim,alert-low-soc-level",
> > > +			&chip->low_soc_alert_threshold)) {
> > 
> > Please align the line break with line above. checkpatch --strict might
> > give you hints about this.
> > >> +		chip->low_soc_alert_threshold = MAX17040_ATHD_DEFAULT_POWER_UP;
> > > +	/* check if low_soc_alert_threshold is between 1% and 32% */
> > 
> > The comment looks misleading here, like it belongs to previous block.
> > Maybe put it inside else if {} block?
> > 
> > > +	} else if (chip->low_soc_alert_threshold <= 0 ||
> > > +			chip->low_soc_alert_threshold >= 33){
> > 
> > Missing space before {.
> > 
> > > +		ret = -EINVAL;
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >   static void max17040_check_changes(struct i2c_client *client)
> > >   {
> > >   	max17040_get_vcell(client);
> > > @@ -192,6 +230,10 @@ static irqreturn_t max17040_thread_handler(int id, void *dev)
> > >   	/* send uevent */
> > >   	power_supply_changed(chip->battery);
> > > 
> > > +	/* reset alert bit */
> > > +	max17040_set_low_soc_threshold_alert(client,
> > > +		chip->low_soc_alert_threshold);
> > 
> > Unless the continuation exceeds 80 character limit, please align it with
> > previous line.
> > 
> > > +
> > >   	return IRQ_HANDLED;
> > >   }
> > > 
> > > @@ -216,6 +258,7 @@ static int max17040_probe(struct i2c_client *client,
> > >   	struct i2c_adapter *adapter = client->adapter;
> > >   	struct power_supply_config psy_cfg = {};
> > >   	struct max17040_chip *chip;
> > > +	int ret;
> > > 
> > >   	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
> > >   		return -EIO;
> > > @@ -226,6 +269,12 @@ static int max17040_probe(struct i2c_client *client,
> > > 
> > >   	chip->client = client;
> > >   	chip->pdata = client->dev.platform_data;
> > > +	ret = max17040_get_of_data(chip);
> > > +	if (ret) {
> > > +		dev_err(&client->dev,
> > > +			"failed: low SOC alert OF data out of bounds\n");
> > > +		return ret;
> > > +	}
> > > 
> > >   	i2c_set_clientdata(client, chip);
> > >   	psy_cfg.drv_data = chip;
> > > @@ -242,20 +291,31 @@ static int max17040_probe(struct i2c_client *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;
> > > +		if (of_device_is_compatible(client->dev.of_node,
> > > +			"maxim,max77836-battery")) {
> > 
> > Alignment.
> > 
> > > +			ret = max17040_set_low_soc_threshold_alert(client,
> > > +				chip->low_soc_alert_threshold);
> > 
> > Ditto.
> > 
> > > +			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");
> > > +			ret = devm_request_threaded_irq(&client->dev,
> > > +				client->irq, NULL, max17040_thread_handler,
> > > +				(client->flags | IRQF_ONESHOT),
> > 
> > This looks unrelated. Befor ethis were IRQF_TRIGGER_FALLING |
> > IRQF_ONESHOT, now you use client->flags. There is no reason why this
> > commit should change >
> 
> I am using client->flags here to not overwrite the flag passed in device
> tree. Let me know what you think about it: if I should leave it as in the
> previous commit, or should I modify the previous commit too.

I still do not get why this change is here and how is related to this
commit.


Best regards,
Krzysztof

[PATCH v4 4/4] power: supply: max17040: Send uevent in SOC and status change

[Matheus Castello]

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 <matheus@castello.eng.br>
---
 drivers/power/supply/max17040_battery.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
index 802575342c72..0ed3d856e4f5 100644
--- a/drivers/power/supply/max17040_battery.c
+++ b/drivers/power/supply/max17040_battery.c
@@ -210,10 +210,19 @@ static void max17040_check_changes(struct i2c_client *client)
 static void max17040_work(struct work_struct *work)
 {
 	struct max17040_chip *chip;
+	int last_soc, last_status;

 	chip = container_of(work, struct max17040_chip, work.work);
+
+	/* store SOC and status to check changes */
+	last_soc = chip->soc;
+	last_status = chip->status;
 	max17040_check_changes(chip->client);

+	/* check changes and send uevent */
+	if (last_soc != chip->soc || last_status != chip->status)
+		power_supply_changed(chip->battery);
+
 	queue_delayed_work(system_power_efficient_wq, &chip->work,
 			   MAX17040_DELAY);
 }
--
2.24.0.rc2

Re: [PATCH v4 4/4] power: supply: max17040: Send uevent in SOC and status change

[Krzysztof Kozlowski]

On Thu, Oct 31, 2019 at 03:41:34PM -0300, Matheus Castello 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 <matheus@castello.eng.br>
> ---
>  drivers/power/supply/max17040_battery.c | 9 +++++++++

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof