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

[PATCH v7 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 Kozlowski, Lee Jones and Rob Herring for your time reviewing
it.

Changes since v6:
(Suggested by Lee Jones)
- Use relative paths in documentation

Changes since v5:
(Suggested by Krzysztof Kozlowski)
- Rearrange code and add max17040_enable_alert_irq on patch 1/5
- Remove useless dev_info

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 v7 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 v7 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>
Reviewed-by: Rob Herring <robh@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 v7 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>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Rob Herring <robh@kernel.org>
---
 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..45c7f414aee0 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:
+power/supply/max17040_battery.txt


 Required properties:
--
2.24.0.rc2

[PATCH v7 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

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

[Sebastian Reichel]

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

Hi,

On Sun, Nov 17, 2019 at 11:13:34AM -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 | 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);
> -

unrelated change.

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

use device_property_read_u32(), which is not DT specific. Also
code can be simplified a bit:

chip->low_soc_alert = MAX17040_ATHD_DEFAULT_POWER_UP;
device_property_read_u32(dev, "maxim,alert-low-soc-level", &chip->low_soc_alert);
if (chip->low_soc_alert <= 0 || chip->low_soc_alert >= 33)
    return -EINVAL;
return 0;

> +
> +	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");

Something is odd here. Either this should be part of the first
patch ("max17040: Add IRQ handler for low SOC alert"), or both
device types support the IRQ and this check should be removed.

-- Sebastian

>  		}
>  	}
> 
> --
> 2.24.0.rc2
> 

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

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

[Matheus Castello]

Hi Sebastian,

Em 11/26/19 11:52 AM, Sebastian Reichel escreveu:
> Hi,
> 
> On Sun, Nov 17, 2019 at 11:13:34AM -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 | 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);
>> -
> 
> unrelated change.
> 
>>   	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;
>> +	}
> 
> use device_property_read_u32(), which is not DT specific. Also
> code can be simplified a bit:
> 
> chip->low_soc_alert = MAX17040_ATHD_DEFAULT_POWER_UP;
> device_property_read_u32(dev, "maxim,alert-low-soc-level", &chip->low_soc_alert);
> if (chip->low_soc_alert <= 0 || chip->low_soc_alert >= 33)
>      return -EINVAL;
> return 0;
> 

Thanks for the review, I will use this.

>> +
>> +	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");
> 
> Something is odd here. Either this should be part of the first
> patch ("max17040: Add IRQ handler for low SOC alert"), or both
> device types support the IRQ and this check should be removed.
> > -- Sebastian
>

The first patch add the IRQ without the configuration of the low SoC 
alert, using the default state of charge level. This patch is working 
with registers to config the low state of charge level, so it was 
proposed to just try to write registers in the models compatible with 
that (maxim,max77836-battery).

Maybe join the first patch to this one, and let DT binding be the first 
patch on the series so we can already test compatible here, let me know 
what you think about it.

>>   		}
>>   	}
>>
>> --
>> 2.24.0.rc2
>>

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

[Sebastian Reichel]

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

Hi,

On Wed, Nov 27, 2019 at 10:06:47PM -0300, Matheus Castello wrote:
> [...]
> > > @@ -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");
> > 
> > Something is odd here. Either this should be part of the first
> > patch ("max17040: Add IRQ handler for low SOC alert"), or both
> > device types support the IRQ and this check should be removed.
> 
> The first patch add the IRQ without the configuration of the low SoC alert,
> using the default state of charge level. This patch is working with
> registers to config the low state of charge level, so it was proposed to
> just try to write registers in the models compatible with that
> (maxim,max77836-battery).
> 
> Maybe join the first patch to this one, and let DT binding be the first
> patch on the series so we can already test compatible here, let me know what
> you think about it.

Assuming the !max77836 do not have any interrupt support, you can
just add the OF check in the first patch in "if (client->irq)", so
that it reads 

if (client->irq && of_device_is_compatible(...)) {
    ...
}

-- Sebastian

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

[PATCH v7 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