diff mbox series

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

Message ID 20191117141335.23404-5-matheus@castello.eng.br (mailing list archive)
State Not Applicable, archived
Headers show
Series power: supply: MAX17040: Add IRQ for low level and alert SOC changes | expand

Commit Message

Matheus Castello Nov. 17, 2019, 2:13 p.m. UTC 
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(-)

--
2.24.0.rc2

Comments

Sebastian Reichel Nov. 26, 2019, 2:52 p.m. UTC | #1 
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
>
Matheus Castello Nov. 28, 2019, 1:06 a.m. UTC | #2 
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
>>
Sebastian Reichel Nov. 29, 2019, 6:33 p.m. UTC | #3 
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
diff mbox series

Patch

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