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

From: Matheus Castello
Date: Wed Nov 27 2019 - 20:06:56 EST

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@xxxxxxxxxxxxxxx>
---
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