linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/2] add Watchdog Timer delay support for BQ24735
@ 2021-07-09 14:27 Bruno Meneguele
  2021-07-09 14:27 ` [PATCH v3 1/2] power: supply: bq24735: reorganize ChargeOption command macros Bruno Meneguele
  2021-07-09 14:27 ` [PATCH v3 2/2] power: supply: bq24735: add watchdog timer delay support Bruno Meneguele
  0 siblings, 2 replies; 6+ messages in thread
From: Bruno Meneguele @ 2021-07-09 14:27 UTC (permalink / raw)
  To: sre; +Cc: linux-pm, linux-kernel

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:
  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: reorganize ChargeOption command macros
  power: supply: bq24735: add watchdog timer delay support

 .../bindings/power/supply/bq24735.yaml        | 13 ++++
 drivers/power/supply/bq24735-charger.c        | 75 ++++++++++++++++---
 include/linux/power/bq24735-charger.h         |  1 +
 3 files changed, 77 insertions(+), 12 deletions(-)

-- 
2.31.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v3 1/2] power: supply: bq24735: reorganize ChargeOption command macros
  2021-07-09 14:27 [PATCH v3 0/2] add Watchdog Timer delay support for BQ24735 Bruno Meneguele
@ 2021-07-09 14:27 ` Bruno Meneguele
  2021-08-13 16:12   ` Sebastian Reichel
  2021-07-09 14:27 ` [PATCH v3 2/2] power: supply: bq24735: add watchdog timer delay support Bruno Meneguele
  1 sibling, 1 reply; 6+ messages in thread
From: Bruno Meneguele @ 2021-07-09 14:27 UTC (permalink / raw)
  To: sre; +Cc: linux-pm, linux-kernel, Bruno Meneguele

Rename ChargeOption macros to match the others for ChargeCurrent and
ChargeVoltage and also separate the command & masks macros from the bits of
interest macros for each command.  This macro doesn't introduce any
functional change, only code re-org.

Signed-off-by: Bruno Meneguele <bruno.meneguele@smartgreen.net>
---
 drivers/power/supply/bq24735-charger.c | 27 ++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/power/supply/bq24735-charger.c b/drivers/power/supply/bq24735-charger.c
index b5d619db79f6..3ce36d09c017 100644
--- a/drivers/power/supply/bq24735-charger.c
+++ b/drivers/power/supply/bq24735-charger.c
@@ -31,9 +31,8 @@
 
 #include <linux/power/bq24735-charger.h>
 
-#define BQ24735_CHG_OPT			0x12
-#define BQ24735_CHG_OPT_CHARGE_DISABLE	(1 << 0)
-#define BQ24735_CHG_OPT_AC_PRESENT	(1 << 4)
+/* BQ24735 available commands and their respective masks */
+#define BQ24735_CHARGE_OPT		0x12
 #define BQ24735_CHARGE_CURRENT		0x14
 #define BQ24735_CHARGE_CURRENT_MASK	0x1fc0
 #define BQ24735_CHARGE_VOLTAGE		0x15
@@ -43,6 +42,10 @@
 #define BQ24735_MANUFACTURER_ID		0xfe
 #define BQ24735_DEVICE_ID		0xff
 
+/* ChargeOptions bits of interest */
+#define BQ24735_CHARGE_OPT_CHG_DISABLE	(1 << 0)
+#define BQ24735_CHARGE_OPT_AC_PRESENT	(1 << 4)
+
 struct bq24735 {
 	struct power_supply		*charger;
 	struct power_supply_desc	charger_desc;
@@ -167,8 +170,8 @@ static inline int bq24735_enable_charging(struct bq24735 *charger)
 	if (ret)
 		return ret;
 
-	return bq24735_update_word(charger->client, BQ24735_CHG_OPT,
-				   BQ24735_CHG_OPT_CHARGE_DISABLE, 0);
+	return bq24735_update_word(charger->client, BQ24735_CHARGE_OPT,
+				   BQ24735_CHARGE_OPT_CHG_DISABLE, 0);
 }
 
 static inline int bq24735_disable_charging(struct bq24735 *charger)
@@ -176,9 +179,9 @@ static inline int bq24735_disable_charging(struct bq24735 *charger)
 	if (charger->pdata->ext_control)
 		return 0;
 
-	return bq24735_update_word(charger->client, BQ24735_CHG_OPT,
-				   BQ24735_CHG_OPT_CHARGE_DISABLE,
-				   BQ24735_CHG_OPT_CHARGE_DISABLE);
+	return bq24735_update_word(charger->client, BQ24735_CHARGE_OPT,
+				   BQ24735_CHARGE_OPT_CHG_DISABLE,
+				   BQ24735_CHARGE_OPT_CHG_DISABLE);
 }
 
 static bool bq24735_charger_is_present(struct bq24735 *charger)
@@ -188,14 +191,14 @@ static bool bq24735_charger_is_present(struct bq24735 *charger)
 	} else {
 		int ac = 0;
 
-		ac = bq24735_read_word(charger->client, BQ24735_CHG_OPT);
+		ac = bq24735_read_word(charger->client, BQ24735_CHARGE_OPT);
 		if (ac < 0) {
 			dev_dbg(&charger->client->dev,
 				"Failed to read charger options : %d\n",
 				ac);
 			return false;
 		}
-		return (ac & BQ24735_CHG_OPT_AC_PRESENT) ? true : false;
+		return (ac & BQ24735_CHARGE_OPT_AC_PRESENT) ? true : false;
 	}
 
 	return false;
@@ -208,11 +211,11 @@ static int bq24735_charger_is_charging(struct bq24735 *charger)
 	if (!bq24735_charger_is_present(charger))
 		return 0;
 
-	ret  = bq24735_read_word(charger->client, BQ24735_CHG_OPT);
+	ret  = bq24735_read_word(charger->client, BQ24735_CHARGE_OPT);
 	if (ret < 0)
 		return ret;
 
-	return !(ret & BQ24735_CHG_OPT_CHARGE_DISABLE);
+	return !(ret & BQ24735_CHARGE_OPT_CHG_DISABLE);
 }
 
 static void bq24735_update(struct bq24735 *charger)
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v3 2/2] power: supply: bq24735: add watchdog timer delay support
  2021-07-09 14:27 [PATCH v3 0/2] add Watchdog Timer delay support for BQ24735 Bruno Meneguele
  2021-07-09 14:27 ` [PATCH v3 1/2] power: supply: bq24735: reorganize ChargeOption command macros Bruno Meneguele
@ 2021-07-09 14:27 ` Bruno Meneguele
  2021-08-13 16:29   ` Sebastian Reichel
  1 sibling, 1 reply; 6+ messages in thread
From: Bruno Meneguele @ 2021-07-09 14:27 UTC (permalink / raw)
  To: sre; +Cc: linux-pm, linux-kernel, 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>
---
 .../bindings/power/supply/bq24735.yaml        | 13 +++++
 drivers/power/supply/bq24735-charger.c        | 48 +++++++++++++++++++
 include/linux/power/bq24735-charger.h         |  1 +
 3 files changed, 62 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/supply/bq24735.yaml b/Documentation/devicetree/bindings/power/supply/bq24735.yaml
index 131be6782c4b..62399efab467 100644
--- a/Documentation/devicetree/bindings/power/supply/bq24735.yaml
+++ b/Documentation/devicetree/bindings/power/supply/bq24735.yaml
@@ -56,6 +56,19 @@ 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
+    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: |
diff --git a/drivers/power/supply/bq24735-charger.c b/drivers/power/supply/bq24735-charger.c
index 3ce36d09c017..88f1cb1e9fee 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) {
+		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,17 @@ 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 <= 3) {
+			pdata->wdt_timeout = val;
+		} else {
+			dev_warn(&client->dev,
+				 "Invalid value for ti,wdt-timeout: %d",
+				 val);
+		}
+	}
+
 	pdata->ext_control = of_property_read_bool(np, "ti,external-control");
 
 	return pdata;
@@ -476,6 +503,27 @@ static int bq24735_charger_probe(struct i2c_client *client,
 			return 0;
 		if (!charger->poll_interval)
 			return 0;
+		if (charger->pdata->wdt_timeout) {
+			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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/2] power: supply: bq24735: reorganize ChargeOption command macros
  2021-07-09 14:27 ` [PATCH v3 1/2] power: supply: bq24735: reorganize ChargeOption command macros Bruno Meneguele
@ 2021-08-13 16:12   ` Sebastian Reichel
  0 siblings, 0 replies; 6+ messages in thread
From: Sebastian Reichel @ 2021-08-13 16:12 UTC (permalink / raw)
  To: Bruno Meneguele; +Cc: linux-pm, linux-kernel

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

Hi,

On Fri, Jul 09, 2021 at 11:27:30AM -0300, Bruno Meneguele wrote:
> Rename ChargeOption macros to match the others for ChargeCurrent and
> ChargeVoltage and also separate the command & masks macros from the bits of
> interest macros for each command.  This macro doesn't introduce any
> functional change, only code re-org.
> 
> Signed-off-by: Bruno Meneguele <bruno.meneguele@smartgreen.net>
> ---

Thanks, queued.

-- Sebastian

>  drivers/power/supply/bq24735-charger.c | 27 ++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/power/supply/bq24735-charger.c b/drivers/power/supply/bq24735-charger.c
> index b5d619db79f6..3ce36d09c017 100644
> --- a/drivers/power/supply/bq24735-charger.c
> +++ b/drivers/power/supply/bq24735-charger.c
> @@ -31,9 +31,8 @@
>  
>  #include <linux/power/bq24735-charger.h>
>  
> -#define BQ24735_CHG_OPT			0x12
> -#define BQ24735_CHG_OPT_CHARGE_DISABLE	(1 << 0)
> -#define BQ24735_CHG_OPT_AC_PRESENT	(1 << 4)
> +/* BQ24735 available commands and their respective masks */
> +#define BQ24735_CHARGE_OPT		0x12
>  #define BQ24735_CHARGE_CURRENT		0x14
>  #define BQ24735_CHARGE_CURRENT_MASK	0x1fc0
>  #define BQ24735_CHARGE_VOLTAGE		0x15
> @@ -43,6 +42,10 @@
>  #define BQ24735_MANUFACTURER_ID		0xfe
>  #define BQ24735_DEVICE_ID		0xff
>  
> +/* ChargeOptions bits of interest */
> +#define BQ24735_CHARGE_OPT_CHG_DISABLE	(1 << 0)
> +#define BQ24735_CHARGE_OPT_AC_PRESENT	(1 << 4)
> +
>  struct bq24735 {
>  	struct power_supply		*charger;
>  	struct power_supply_desc	charger_desc;
> @@ -167,8 +170,8 @@ static inline int bq24735_enable_charging(struct bq24735 *charger)
>  	if (ret)
>  		return ret;
>  
> -	return bq24735_update_word(charger->client, BQ24735_CHG_OPT,
> -				   BQ24735_CHG_OPT_CHARGE_DISABLE, 0);
> +	return bq24735_update_word(charger->client, BQ24735_CHARGE_OPT,
> +				   BQ24735_CHARGE_OPT_CHG_DISABLE, 0);
>  }
>  
>  static inline int bq24735_disable_charging(struct bq24735 *charger)
> @@ -176,9 +179,9 @@ static inline int bq24735_disable_charging(struct bq24735 *charger)
>  	if (charger->pdata->ext_control)
>  		return 0;
>  
> -	return bq24735_update_word(charger->client, BQ24735_CHG_OPT,
> -				   BQ24735_CHG_OPT_CHARGE_DISABLE,
> -				   BQ24735_CHG_OPT_CHARGE_DISABLE);
> +	return bq24735_update_word(charger->client, BQ24735_CHARGE_OPT,
> +				   BQ24735_CHARGE_OPT_CHG_DISABLE,
> +				   BQ24735_CHARGE_OPT_CHG_DISABLE);
>  }
>  
>  static bool bq24735_charger_is_present(struct bq24735 *charger)
> @@ -188,14 +191,14 @@ static bool bq24735_charger_is_present(struct bq24735 *charger)
>  	} else {
>  		int ac = 0;
>  
> -		ac = bq24735_read_word(charger->client, BQ24735_CHG_OPT);
> +		ac = bq24735_read_word(charger->client, BQ24735_CHARGE_OPT);
>  		if (ac < 0) {
>  			dev_dbg(&charger->client->dev,
>  				"Failed to read charger options : %d\n",
>  				ac);
>  			return false;
>  		}
> -		return (ac & BQ24735_CHG_OPT_AC_PRESENT) ? true : false;
> +		return (ac & BQ24735_CHARGE_OPT_AC_PRESENT) ? true : false;
>  	}
>  
>  	return false;
> @@ -208,11 +211,11 @@ static int bq24735_charger_is_charging(struct bq24735 *charger)
>  	if (!bq24735_charger_is_present(charger))
>  		return 0;
>  
> -	ret  = bq24735_read_word(charger->client, BQ24735_CHG_OPT);
> +	ret  = bq24735_read_word(charger->client, BQ24735_CHARGE_OPT);
>  	if (ret < 0)
>  		return ret;
>  
> -	return !(ret & BQ24735_CHG_OPT_CHARGE_DISABLE);
> +	return !(ret & BQ24735_CHARGE_OPT_CHG_DISABLE);
>  }
>  
>  static void bq24735_update(struct bq24735 *charger)
> -- 
> 2.31.1
> 

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 2/2] power: supply: bq24735: add watchdog timer delay support
  2021-07-09 14:27 ` [PATCH v3 2/2] power: supply: bq24735: add watchdog timer delay support Bruno Meneguele
@ 2021-08-13 16:29   ` Sebastian Reichel
  2021-08-16 16:19     ` Bruno Meneguele
  0 siblings, 1 reply; 6+ messages in thread
From: Sebastian Reichel @ 2021-08-13 16:29 UTC (permalink / raw)
  To: Bruno Meneguele; +Cc: linux-pm, linux-kernel

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

Hi,

On Fri, Jul 09, 2021 at 11:27:31AM -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>
> ---
>  .../bindings/power/supply/bq24735.yaml        | 13 +++++

Patches for the DT bindings needs to be CC'd to the DT binding
maintainers and should be in their own patch.

>  drivers/power/supply/bq24735-charger.c        | 48 +++++++++++++++++++
>  include/linux/power/bq24735-charger.h         |  1 +
>  3 files changed, 62 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/bq24735.yaml b/Documentation/devicetree/bindings/power/supply/bq24735.yaml
> index 131be6782c4b..62399efab467 100644
> --- a/Documentation/devicetree/bindings/power/supply/bq24735.yaml
> +++ b/Documentation/devicetree/bindings/power/supply/bq24735.yaml
> @@ -56,6 +56,19 @@ 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
> +    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.
> +

This is missing 

minimum: 0
maximum: 3

>    ti,external-control:
>      type: boolean
>      description: |
> diff --git a/drivers/power/supply/bq24735-charger.c b/drivers/power/supply/bq24735-charger.c
> index 3ce36d09c017..88f1cb1e9fee 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) {
> +		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;
> +		}
> +	}

binding says '0' = disabled, but code implements '0' = do not
change.

-- Sebastian

> +
>  	return 0;
>  }
>  
> @@ -347,6 +363,17 @@ 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 <= 3) {
> +			pdata->wdt_timeout = val;
> +		} else {
> +			dev_warn(&client->dev,
> +				 "Invalid value for ti,wdt-timeout: %d",
> +				 val);
> +		}
> +	}
> +
>  	pdata->ext_control = of_property_read_bool(np, "ti,external-control");
>  
>  	return pdata;
> @@ -476,6 +503,27 @@ static int bq24735_charger_probe(struct i2c_client *client,
>  			return 0;
>  		if (!charger->poll_interval)
>  			return 0;
> +		if (charger->pdata->wdt_timeout) {
> +			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
> 

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 2/2] power: supply: bq24735: add watchdog timer delay support
  2021-08-13 16:29   ` Sebastian Reichel
@ 2021-08-16 16:19     ` Bruno Meneguele
  0 siblings, 0 replies; 6+ messages in thread
From: Bruno Meneguele @ 2021-08-16 16:19 UTC (permalink / raw)
  To: Sebastian Reichel; +Cc: linux-pm, linux-kernel

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

On Fri, Aug 13, 2021 at 06:29:14PM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Fri, Jul 09, 2021 at 11:27:31AM -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>
> > ---
> >  .../bindings/power/supply/bq24735.yaml        | 13 +++++
> 
> Patches for the DT bindings needs to be CC'd to the DT binding
> maintainers and should be in their own patch.
> 

Sorry about that, didn't know.
Will do that in v4.

> >  drivers/power/supply/bq24735-charger.c        | 48 +++++++++++++++++++
> >  include/linux/power/bq24735-charger.h         |  1 +
> >  3 files changed, 62 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/power/supply/bq24735.yaml b/Documentation/devicetree/bindings/power/supply/bq24735.yaml
> > index 131be6782c4b..62399efab467 100644
> > --- a/Documentation/devicetree/bindings/power/supply/bq24735.yaml
> > +++ b/Documentation/devicetree/bindings/power/supply/bq24735.yaml
> > @@ -56,6 +56,19 @@ 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
> > +    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.
> > +
> 
> This is missing 
> 
> minimum: 0
> maximum: 3
> 

Indeed. Will fix in v4.

> >    ti,external-control:
> >      type: boolean
> >      description: |
> > diff --git a/drivers/power/supply/bq24735-charger.c b/drivers/power/supply/bq24735-charger.c
> > index 3ce36d09c017..88f1cb1e9fee 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) {
> > +		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;
> > +		}
> > +	}
> 
> binding says '0' = disabled, but code implements '0' = do not
> change.
> 

Well noticed. Will fix in v4.

I'm going to send a v4 without patch 1/2 because I noticed you already
queued for-next, so the next version should have only 1/2 heandling the
code and 2/2 specifically for dt-bindings.


-- 
Bruno Meneguele
PGP Key: http://bmeneg.com/pubkey.txt

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-08-16 17:18 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-09 14:27 [PATCH v3 0/2] add Watchdog Timer delay support for BQ24735 Bruno Meneguele
2021-07-09 14:27 ` [PATCH v3 1/2] power: supply: bq24735: reorganize ChargeOption command macros Bruno Meneguele
2021-08-13 16:12   ` Sebastian Reichel
2021-07-09 14:27 ` [PATCH v3 2/2] power: supply: bq24735: add watchdog timer delay support Bruno Meneguele
2021-08-13 16:29   ` Sebastian Reichel
2021-08-16 16:19     ` Bruno Meneguele

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).