[PATCH v4 0/2] add Watchdog Timer delay support for BQ24735

[PATCH v4 0/2] add Watchdog Timer delay support for BQ24735

[Bruno Meneguele]

The IC BQ24735 has the ability to suspend the battery charging in case the
system freezes for some reason: the IC observes consecutive writes for
either CargeCurrent of ChargVoltage registers in a maximum period of time.

This period of time can be configured by the user through the ChargeOption
register in the bits 13 and 14, but it's only possible to change if the user
sends the value directly accessing the I2C bus through userspace, because
the kernel driver doesn't read or write to the Watchdog bits.

This patchset enables the user to configure the value through the
device-tree option "ti,wdt-timeout".

Changelog:
  v3 - add specific patch for the dt bidings change.
     - patch 1/2 was already queued to the tree, so it's not present in this
	   patchset anymore.
  v2 - unfortunately I used a default gitconfig that was pointing to my
  default user.email and email smtp. This new version corrects it.

Bruno Meneguele (2):
  power: supply: bq24735: add watchdog timer delay support
  dt-bindings: power: supply: bq24735: document the watchdog timer delay
    feature

 .../bindings/power/supply/bq24735.yaml        | 15 ++++++
 drivers/power/supply/bq24735-charger.c        | 54 +++++++++++++++++++
 include/linux/power/bq24735-charger.h         |  1 +
 3 files changed, 70 insertions(+)

-- 
2.31.1

[PATCH v4 1/2] power: supply: bq24735: add watchdog timer delay support

[Bruno Meneguele]

The BQ24735 charger allows the user to set the watchdog timer delay between
two consecutives ChargeCurrent or ChargeVoltage command writes, if the IC
doesn't receive any command before the timeout happens, the charge is turned
off.

This patch adds the support to the user to change the default/POR value with
four discrete values:

  0 - disabled
  1 - enabled, 44 sec
  2 - enabled, 88 sec
  3 - enabled, 175 sec (default at POR)

These are the options supported in the ChargeOptions register bits 13&14.

Also, this patch make one additional check when poll-interval is set by the
user: if the interval set is greater than the WDT timeout it'll fail during
the probe stage, preventing the user to set non-compatible values between
the two options.

Signed-off-by: Bruno Meneguele <bruno.meneguele@smartgreen.net>
---
Changelog:
  v3 - check wdt_timeout for the maximum and minimum available values 

 drivers/power/supply/bq24735-charger.c | 54 ++++++++++++++++++++++++++
 include/linux/power/bq24735-charger.h  |  1 +
 2 files changed, 55 insertions(+)

diff --git a/drivers/power/supply/bq24735-charger.c b/drivers/power/supply/bq24735-charger.c
index 3ce36d09c017..7e8d0c23df9a 100644
--- a/drivers/power/supply/bq24735-charger.c
+++ b/drivers/power/supply/bq24735-charger.c
@@ -45,6 +45,8 @@
 /* ChargeOptions bits of interest */
 #define BQ24735_CHARGE_OPT_CHG_DISABLE	(1 << 0)
 #define BQ24735_CHARGE_OPT_AC_PRESENT	(1 << 4)
+#define BQ24735_CHARGE_OPT_WDT_OFFSET	13
+#define BQ24735_CHARGE_OPT_WDT		(3 << BQ24735_CHARGE_OPT_WDT_OFFSET)
 
 struct bq24735 {
 	struct power_supply		*charger;
@@ -156,6 +158,20 @@ static int bq24735_config_charger(struct bq24735 *charger)
 		}
 	}
 
+	if (pdata->wdt_timeout >= 0 && pdata->wdt_timeout <= 3) {
+		value = pdata->wdt_timeout;
+
+		ret = bq24735_update_word(charger->client, BQ24735_CHARGE_OPT,
+					  BQ24735_CHARGE_OPT_WDT,
+					  (value << BQ24735_CHARGE_OPT_WDT_OFFSET));
+		if (ret < 0) {
+			dev_err(&charger->client->dev,
+				"Failed to write watchdog timer: %d\n",
+				ret);
+			return ret;
+		}
+	}
+
 	return 0;
 }
 
@@ -347,6 +363,23 @@ static struct bq24735_platform *bq24735_parse_dt_data(struct i2c_client *client)
 	if (!ret)
 		pdata->input_current = val;
 
+	ret = of_property_read_u32(np, "ti,wdt-timeout", &val);
+	if (!ret) {
+		if (val >= 0 && val <= 3) {
+			pdata->wdt_timeout = val;
+		} else {
+			dev_warn(&client->dev,
+				 "Invalid value for ti,wdt-timeout: %d",
+				 val);
+		}
+	} else {
+		/* Since 0 is a valid value (disabled), set something
+		 * greater than the maximum limit accepted from the user to
+		 * represent the "no change" state. */
+		pdata->wdt_timeout = 4;
+	}
+
+
 	pdata->ext_control = of_property_read_bool(np, "ti,external-control");
 
 	return pdata;
@@ -476,6 +509,27 @@ static int bq24735_charger_probe(struct i2c_client *client,
 			return 0;
 		if (!charger->poll_interval)
 			return 0;
+		if (charger->pdata->wdt_timeout > 0) {
+			int wdt_ms;
+
+			switch (charger->pdata->wdt_timeout) {
+			case 1:
+				wdt_ms = 44000;
+				break;
+			case 2:
+				wdt_ms = 88000;
+				break;
+			case 3:
+				wdt_ms = 175000;
+				break;
+			}
+
+			if (charger->poll_interval > wdt_ms) {
+				dev_err(&client->dev,
+					"Poll interval greater than WDT timeout\n");
+				return -EINVAL;
+			}
+		}
 
 		ret = devm_delayed_work_autocancel(&client->dev, &charger->poll,
 						   bq24735_poll);
diff --git a/include/linux/power/bq24735-charger.h b/include/linux/power/bq24735-charger.h
index 321dd009ce66..ce5a030ca111 100644
--- a/include/linux/power/bq24735-charger.h
+++ b/include/linux/power/bq24735-charger.h
@@ -12,6 +12,7 @@ struct bq24735_platform {
 	uint32_t charge_current;
 	uint32_t charge_voltage;
 	uint32_t input_current;
+	uint32_t wdt_timeout;
 
 	const char *name;
 
-- 
2.31.1

[PATCH v4 2/2] dt-bindings: power: supply: bq24735: document the watchdog timer delay feature

[Bruno Meneguele]

The new watchdog timer delay support in BQ24735 allow the user to set four
different options, ranging from 0 to 3. With that, add this new property and
its values and description to the BQ24735 DT binding documentation.

Signed-off-by: Bruno Meneguele <bruno.meneguele@smartgreen.net>
---
Changelog:
  v3 - create specific patch for dt bindings changes
     - add minimum and maximum values

 .../devicetree/bindings/power/supply/bq24735.yaml | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/supply/bq24735.yaml b/Documentation/devicetree/bindings/power/supply/bq24735.yaml
index 131be6782c4b..93a125359ec7 100644
--- a/Documentation/devicetree/bindings/power/supply/bq24735.yaml
+++ b/Documentation/devicetree/bindings/power/supply/bq24735.yaml
@@ -56,6 +56,21 @@ properties:
       The POR value is 0x1000h. This number is in mA (e.g. 8064).
       See the spec for more information about the InputCurrent (0x3fh) register.
 
+  ti,wdt-timeout:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 3
+    description: |
+      Used to control and set the charger watchdog delay between consecutive
+      charge voltage and charge current commands.
+      This value must be:
+        0 - disabled
+        1 - 44 seconds
+        2 - 88 seconds
+        3 - 175 seconds
+      The POR value is 0x11 (3).
+      See the spec for more information about the ChargeOptions(0x12h) register.
+
   ti,external-control:
     type: boolean
     description: |
-- 
2.31.1

Re: [PATCH v4 2/2] dt-bindings: power: supply: bq24735: document the watchdog timer delay feature

[Rob Herring]

On Mon, 16 Aug 2021 13:52:45 -0300, Bruno Meneguele wrote:
> The new watchdog timer delay support in BQ24735 allow the user to set four
> different options, ranging from 0 to 3. With that, add this new property and
> its values and description to the BQ24735 DT binding documentation.
> 
> Signed-off-by: Bruno Meneguele <bruno.meneguele@smartgreen.net>
> ---
> Changelog:
>   v3 - create specific patch for dt bindings changes
>      - add minimum and maximum values
> 
>  .../devicetree/bindings/power/supply/bq24735.yaml | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 

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

Re: [PATCH v4 1/2] power: supply: bq24735: add watchdog timer delay support

[Sebastian Reichel]

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

Hi,

On Mon, Aug 16, 2021 at 01:52:44PM -0300, Bruno Meneguele wrote:
> The BQ24735 charger allows the user to set the watchdog timer delay between
> two consecutives ChargeCurrent or ChargeVoltage command writes, if the IC
> doesn't receive any command before the timeout happens, the charge is turned
> off.
> 
> This patch adds the support to the user to change the default/POR value with
> four discrete values:
> 
>   0 - disabled
>   1 - enabled, 44 sec
>   2 - enabled, 88 sec
>   3 - enabled, 175 sec (default at POR)
> 
> These are the options supported in the ChargeOptions register bits 13&14.
> 
> Also, this patch make one additional check when poll-interval is set by the
> user: if the interval set is greater than the WDT timeout it'll fail during
> the probe stage, preventing the user to set non-compatible values between
> the two options.
> 
> Signed-off-by: Bruno Meneguele <bruno.meneguele@smartgreen.net>
> ---
> Changelog:
>   v3 - check wdt_timeout for the maximum and minimum available values 
> 
>  drivers/power/supply/bq24735-charger.c | 54 ++++++++++++++++++++++++++
>  include/linux/power/bq24735-charger.h  |  1 +
>  2 files changed, 55 insertions(+)
> 
> diff --git a/drivers/power/supply/bq24735-charger.c b/drivers/power/supply/bq24735-charger.c
> index 3ce36d09c017..7e8d0c23df9a 100644
> --- a/drivers/power/supply/bq24735-charger.c
> +++ b/drivers/power/supply/bq24735-charger.c
> @@ -45,6 +45,8 @@
>  /* ChargeOptions bits of interest */
>  #define BQ24735_CHARGE_OPT_CHG_DISABLE	(1 << 0)
>  #define BQ24735_CHARGE_OPT_AC_PRESENT	(1 << 4)
> +#define BQ24735_CHARGE_OPT_WDT_OFFSET	13
> +#define BQ24735_CHARGE_OPT_WDT		(3 << BQ24735_CHARGE_OPT_WDT_OFFSET)
>  
>  struct bq24735 {
>  	struct power_supply		*charger;
> @@ -156,6 +158,20 @@ static int bq24735_config_charger(struct bq24735 *charger)
>  		}
>  	}
>  
> +	if (pdata->wdt_timeout >= 0 && pdata->wdt_timeout <= 3) {

wdt_timeout is unsigned and can't be smaller than 0.

> +		value = pdata->wdt_timeout;
> +
> +		ret = bq24735_update_word(charger->client, BQ24735_CHARGE_OPT,
> +					  BQ24735_CHARGE_OPT_WDT,
> +					  (value << BQ24735_CHARGE_OPT_WDT_OFFSET));
> +		if (ret < 0) {
> +			dev_err(&charger->client->dev,
> +				"Failed to write watchdog timer: %d\n",
> +				ret);
> +			return ret;
> +		}
> +	}
> +
>  	return 0;
>  }
>  
> @@ -347,6 +363,23 @@ static struct bq24735_platform *bq24735_parse_dt_data(struct i2c_client *client)
>  	if (!ret)
>  		pdata->input_current = val;
>  
> +	ret = of_property_read_u32(np, "ti,wdt-timeout", &val);
> +	if (!ret) {
> +		if (val >= 0 && val <= 3) {

wdt_timeout is unsigned and can't be smaller than 0.

> +			pdata->wdt_timeout = val;
> +		} else {
> +			dev_warn(&client->dev,
> +				 "Invalid value for ti,wdt-timeout: %d",
> +				 val);
> +		}
> +	} else {
> +		/* Since 0 is a valid value (disabled), set something
> +		 * greater than the maximum limit accepted from the user to
> +		 * represent the "no change" state. */
> +		pdata->wdt_timeout = 4;
> +	}
> +
> +
>  	pdata->ext_control = of_property_read_bool(np, "ti,external-control");
>  
>  	return pdata;
> @@ -476,6 +509,27 @@ static int bq24735_charger_probe(struct i2c_client *client,
>  			return 0;
>  		if (!charger->poll_interval)
>  			return 0;
> +		if (charger->pdata->wdt_timeout > 0) {
> +			int wdt_ms;
> +
> +			switch (charger->pdata->wdt_timeout) {
> +			case 1:
> +				wdt_ms = 44000;
> +				break;
> +			case 2:
> +				wdt_ms = 88000;
> +				break;
> +			case 3:
> +				wdt_ms = 175000;
> +				break;
> +			}

wdt_ms is not initialized when wdt_timeout is not 1-3 resulting in undefined behaviour.
Also please create constants for the magic numbers, e.g.

#define BQ24735_WDT_OFF                 0
#define BQ24735_WDT_44_SEC              1
#define BQ24735_WDT_88_SEC              2
#define BQ24735_WDT_175_SEC             3
#define BQ24735_WDT_INVALID             4

and use them throughout the code.

-- Sebastian

> +
> +			if (charger->poll_interval > wdt_ms) {
> +				dev_err(&client->dev,
> +					"Poll interval greater than WDT timeout\n");
> +				return -EINVAL;
> +			}
> +		}
>  
>  		ret = devm_delayed_work_autocancel(&client->dev, &charger->poll,
>  						   bq24735_poll);
> diff --git a/include/linux/power/bq24735-charger.h b/include/linux/power/bq24735-charger.h
> index 321dd009ce66..ce5a030ca111 100644
> --- a/include/linux/power/bq24735-charger.h
> +++ b/include/linux/power/bq24735-charger.h
> @@ -12,6 +12,7 @@ struct bq24735_platform {
>  	uint32_t charge_current;
>  	uint32_t charge_voltage;
>  	uint32_t input_current;
> +	uint32_t wdt_timeout;
>  
>  	const char *name;
>  
> -- 
> 2.31.1
> 

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