LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v3 0/3] Add support for CellWise cw2015 fuel gauge
@ 2020-03-11  9:30 Tobias Schramm
  2020-03-11  9:30 ` [PATCH v3 1/3] dt-bindings: Document cellwise vendor-prefix Tobias Schramm
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Tobias Schramm @ 2020-03-11  9:30 UTC (permalink / raw)
  To: Andy Shevchenko, Sebastian Reichel, Rob Herring, Mark Rutland
  Cc: Maxime Ripard, Sam Ravnborg, Heiko Stuebner, Stephan Gerhold,
	Mark Brown, Mauro Carvalho Chehab, David S. Miller,
	Greg Kroah-Hartman, Jonathan Cameron, linux-pm, devicetree,
	linux-kernel, Tobias Schramm

This patchset adds support for the CellWise cw2015 fuel gauge.

The CellWise cw2015 fuel gauge is a shuntless, single-cell Li-Ion fuel
gauge. It is used in the pine64 Pinebook Pro laptop.

This is v3 of the patchset. This version incorporates a review by Andy and
includes a commit documenting the cellwise vendor prefix which I forgot to
send in v2.

I've kept the cellwise,battery-profile property in the device tree. Its
content describes characteristics of the battery built into a device. The
exact format is unknown and not publicly documented. It is likely
comprised of some key parameters of the battery (chemistry, voltages,
design capacity) and parameters for tuning the internal state of charge
approximation function.
Since v2 CellWise has confirmed to me that the only way to obtain the
profile blob is to mail them batteries for testing. Thus we will need to
keep that property.

In general I'm not 100 % sure about my json-schema binding for the gauge.
It is my first time ever writing a json-schema binding and I'm not sure
whether properties like power-supplies or monitored-battery need to be
added to a separate, common schema for power supplies or not.


Best Regards,

Tobias Schramm

Changelog:
 v2:
  * Change subject to "Add support for CellWise cw2015 fuel gauge"
  * Rewrite bindings as json-schema
  * Use default power-supplies handling
  * Use regmap for register access
  * Use standard simple-battery node
  * Replace printk/pr_* by dev_{dbg,info,warn,err}
  * Use cancel_delayed_work_sync in remove
  * General code cleanup
 v3:
  * Incorporate review by Andy
  * Add cellwise vendor prefix
  * Rename cellwise,bat-config-info property to cellwise,battery-profile
  * Remove most state of charge post-processing
  * Use fwnode interface
  * General code cleanup
  * Lots of code style fixes

Tobias Schramm (3):
  dt-bindings: Document cellwise vendor-prefix
  dt-bindings: power: supply: add cw2015_battery bindings
  power: supply: add CellWise cw2015 fuel gauge driver

 .../bindings/power/supply/cw2015_battery.yaml |  83 ++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   6 +
 drivers/power/supply/Kconfig                  |   8 +
 drivers/power/supply/Makefile                 |   1 +
 drivers/power/supply/cw2015_battery.c         | 785 ++++++++++++++++++
 6 files changed, 885 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/cw2015_battery.yaml
 create mode 100644 drivers/power/supply/cw2015_battery.c

-- 
2.24.1


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

* [PATCH v3 1/3] dt-bindings: Document cellwise vendor-prefix
  2020-03-11  9:30 [PATCH v3 0/3] Add support for CellWise cw2015 fuel gauge Tobias Schramm
@ 2020-03-11  9:30 ` Tobias Schramm
  2020-03-11  9:30 ` [PATCH v3 2/3] dt-bindings: power: supply: add cw2015_battery bindings Tobias Schramm
  2020-03-11  9:30 ` [PATCH v3 3/3] power: supply: add CellWise cw2015 fuel gauge driver Tobias Schramm
  2 siblings, 0 replies; 10+ messages in thread
From: Tobias Schramm @ 2020-03-11  9:30 UTC (permalink / raw)
  To: Andy Shevchenko, Sebastian Reichel, Rob Herring, Mark Rutland
  Cc: Maxime Ripard, Sam Ravnborg, Heiko Stuebner, Stephan Gerhold,
	Mark Brown, Mauro Carvalho Chehab, David S. Miller,
	Greg Kroah-Hartman, Jonathan Cameron, linux-pm, devicetree,
	linux-kernel, Tobias Schramm

Signed-off-by: Tobias Schramm <t.schramm@manjaro.org>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 9e67944bec9c..af3d1aed8118 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -177,6 +177,8 @@ patternProperties:
     description: Cadence Design Systems Inc.
   "^cdtech,.*":
     description: CDTech(H.K.) Electronics Limited
+  "^cellwise,.*":
+    description: CellWise Microelectronics Co., Ltd
   "^ceva,.*":
     description: Ceva, Inc.
   "^chipidea,.*":
-- 
2.24.1


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

* [PATCH v3 2/3] dt-bindings: power: supply: add cw2015_battery bindings
  2020-03-11  9:30 [PATCH v3 0/3] Add support for CellWise cw2015 fuel gauge Tobias Schramm
  2020-03-11  9:30 ` [PATCH v3 1/3] dt-bindings: Document cellwise vendor-prefix Tobias Schramm
@ 2020-03-11  9:30 ` Tobias Schramm
  2020-03-11 17:20   ` Daniel Thompson
  2020-03-11  9:30 ` [PATCH v3 3/3] power: supply: add CellWise cw2015 fuel gauge driver Tobias Schramm
  2 siblings, 1 reply; 10+ messages in thread
From: Tobias Schramm @ 2020-03-11  9:30 UTC (permalink / raw)
  To: Andy Shevchenko, Sebastian Reichel, Rob Herring, Mark Rutland
  Cc: Maxime Ripard, Sam Ravnborg, Heiko Stuebner, Stephan Gerhold,
	Mark Brown, Mauro Carvalho Chehab, David S. Miller,
	Greg Kroah-Hartman, Jonathan Cameron, linux-pm, devicetree,
	linux-kernel, Tobias Schramm

This patch adds the dts binding schema for the cw2015 fuel gauge.

Signed-off-by: Tobias Schramm <t.schramm@manjaro.org>
---
 .../bindings/power/supply/cw2015_battery.yaml | 83 +++++++++++++++++++
 1 file changed, 83 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/cw2015_battery.yaml

diff --git a/Documentation/devicetree/bindings/power/supply/cw2015_battery.yaml b/Documentation/devicetree/bindings/power/supply/cw2015_battery.yaml
new file mode 100644
index 000000000000..647dbc6e136e
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/cw2015_battery.yaml
@@ -0,0 +1,83 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/supply/cw2015_battery.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Battery driver for CW2015 shuntless fule gauge by CellWise.
+
+maintainers:
+  - Tobias Schramm <t.schramm@manjaro.org>
+
+description: |
+  The driver can utilize information from a simple-battery linked via a
+  phandle in monitored-battery. If specified the driver uses the
+  charge-full-design-microamp-hours property of the battery.
+
+properties:
+  compatible:
+    const: cellwise,cw2015
+
+  reg:
+    items:
+      - description: i2c address
+
+  cellwise,battery-profile:
+    description: |
+      This property specifies characteristics of the battery used. The format
+      of this binary blob is kept secret by CellWise. The only way to obtain
+      it is to mail two batteries to a test facility of CellWise and receive
+      back a test report with the binary blob.
+    allOf:
+      - $ref: /schemas/types.yaml#definitions/uint8-array
+    items:
+      - minItems: 64
+        maxItems: 64
+
+  cellwise,monitor-interval-ms:
+    description:
+      Specifies the interval in milliseconds gauge values are polled at
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  power-supplies:
+    description:
+      Specifies supplies used for charging the battery connected to this gauge
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/phandle-array
+      - minItems: 1
+        maxItems: 8 # Should be enough
+
+  monitored-battery:
+    description:
+      Specifies the phandle of a simple-battery connected to this gauge
+    $ref: /schemas/types.yaml#/definitions/phandle
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        cw2015@62 {
+            compatible = "cellwise,cw201x";
+            reg = <0x62>;
+            cellwise,battery-profile = /bits/ 8 <
+                0x17 0x67 0x80 0x73 0x6E 0x6C 0x6B 0x63
+                0x77 0x51 0x5C 0x58 0x50 0x4C 0x48 0x36
+                0x15 0x0C 0x0C 0x19 0x5B 0x7D 0x6F 0x69
+                0x69 0x5B 0x0C 0x29 0x20 0x40 0x52 0x59
+                0x57 0x56 0x54 0x4F 0x3B 0x1F 0x7F 0x17
+                0x06 0x1A 0x30 0x5A 0x85 0x93 0x96 0x2D
+                0x48 0x77 0x9C 0xB3 0x80 0x52 0x94 0xCB
+                0x2F 0x00 0x64 0xA5 0xB5 0x11 0xF0 0x11
+           >;
+           cellwise,monitor-interval-ms = <5000>;
+           monitored-battery = <&bat>;
+           power-supplies = <&mains_charger>, <&usb_charger>;
+       };
+    };
+
-- 
2.24.1


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

* [PATCH v3 3/3] power: supply: add CellWise cw2015 fuel gauge driver
  2020-03-11  9:30 [PATCH v3 0/3] Add support for CellWise cw2015 fuel gauge Tobias Schramm
  2020-03-11  9:30 ` [PATCH v3 1/3] dt-bindings: Document cellwise vendor-prefix Tobias Schramm
  2020-03-11  9:30 ` [PATCH v3 2/3] dt-bindings: power: supply: add cw2015_battery bindings Tobias Schramm
@ 2020-03-11  9:30 ` Tobias Schramm
  2020-03-11 10:18   ` Andy Shevchenko
  2 siblings, 1 reply; 10+ messages in thread
From: Tobias Schramm @ 2020-03-11  9:30 UTC (permalink / raw)
  To: Andy Shevchenko, Sebastian Reichel, Rob Herring, Mark Rutland
  Cc: Maxime Ripard, Sam Ravnborg, Heiko Stuebner, Stephan Gerhold,
	Mark Brown, Mauro Carvalho Chehab, David S. Miller,
	Greg Kroah-Hartman, Jonathan Cameron, linux-pm, devicetree,
	linux-kernel, Tobias Schramm

This patch adds a driver for the CellWise cw2015 fuel gauge.

The CellWise cw2015 is a shuntless, single-cell Li-Ion fuel gauge used
in the pine64 Pinebook Pro laptop and some Raspberry Pi UPS HATs.

Signed-off-by: Tobias Schramm <t.schramm@manjaro.org>
---
 MAINTAINERS                           |   6 +
 drivers/power/supply/Kconfig          |   8 +
 drivers/power/supply/Makefile         |   1 +
 drivers/power/supply/cw2015_battery.c | 785 ++++++++++++++++++++++++++
 4 files changed, 800 insertions(+)
 create mode 100644 drivers/power/supply/cw2015_battery.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a6fbdf354d34..0260c89618f9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3885,6 +3885,12 @@ F:	arch/powerpc/include/uapi/asm/spu*.h
 F:	arch/powerpc/oprofile/*cell*
 F:	arch/powerpc/platforms/cell/
 
+CELLWISE CW2015 BATTERY DRIVER
+M:	Tobias Schrammm <t.schramm@manjaro.org>
+S:	Maintained
+F:	Documentation/devicetree/bindings/power/supply/cw2015_battery.yaml
+F:	drivers/power/supply/cw2015_battery.c
+
 CEPH COMMON CODE (LIBCEPH)
 M:	Ilya Dryomov <idryomov@gmail.com>
 M:	Jeff Layton <jlayton@kernel.org>
diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index 9a5591ab90d0..ac905ce60e47 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -116,6 +116,14 @@ config BATTERY_CPCAP
 	  Say Y here to enable support for battery on Motorola
 	  phones and tablets such as droid 4.
 
+config BATTERY_CW2015
+	tristate "CW2015 Battery driver"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  Say Y here to enable support for the cellwise cw2015
+	  battery fuel gauge (used in the Pinebook Pro & others)
+
 config BATTERY_DS2760
 	tristate "DS2760 battery driver (HP iPAQ & others)"
 	depends on W1
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index 6c7da920ea83..69727a10e835 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_BATTERY_ACT8945A)	+= act8945a_charger.o
 obj-$(CONFIG_BATTERY_AXP20X)	+= axp20x_battery.o
 obj-$(CONFIG_CHARGER_AXP20X)	+= axp20x_ac_power.o
 obj-$(CONFIG_BATTERY_CPCAP)	+= cpcap-battery.o
+obj-$(CONFIG_BATTERY_CW2015)	+= cw2015_battery.o
 obj-$(CONFIG_BATTERY_DS2760)	+= ds2760_battery.o
 obj-$(CONFIG_BATTERY_DS2780)	+= ds2780_battery.o
 obj-$(CONFIG_BATTERY_DS2781)	+= ds2781_battery.o
diff --git a/drivers/power/supply/cw2015_battery.c b/drivers/power/supply/cw2015_battery.c
new file mode 100644
index 000000000000..14b1bd687643
--- /dev/null
+++ b/drivers/power/supply/cw2015_battery.c
@@ -0,0 +1,785 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Fuel gauge driver for CellWise 2013 / 2015
+ *
+ * Copyright (C) 2012, RockChip
+ * Copyright (C) 2020, Tobias Schramm
+ *
+ * Authors: xuhuicong <xhc@rock-chips.com>
+ * Authors: Tobias Schramm <t.schramm@manjaro.org>
+ */
+
+#include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/gpio/consumer.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/time.h>
+#include <linux/timekeeping.h>
+#include <linux/workqueue.h>
+
+#define CW2015_SIZE_BATINFO		64
+
+#define CW2015_READ_TRIES		30
+#define CW2015_RESET_TRIES		5
+
+#define CW2015_REG_VERSION		0x00
+#define CW2015_REG_VCELL		0x02
+#define CW2015_REG_SOC			0x04
+#define CW2015_REG_RRT_ALERT		0x06
+#define CW2015_REG_CONFIG		0x08
+#define CW2015_REG_MODE			0x0A
+#define CW2015_REG_BATINFO		0x10
+
+#define CW2015_MODE_SLEEP_MASK		GENMASK(7, 6)
+#define CW2015_MODE_SLEEP		(0x03 << 6)
+#define CW2015_MODE_NORMAL		(0x00 << 6)
+#define CW2015_MODE_QUICK_START		(0x03 << 4)
+#define CW2015_MODE_RESTART		(0x0f << 0)
+
+#define CW2015_CONFIG_UPDATE_FLG	(0x01 << 1)
+#define CW2015_ATHD(x)			((x) << 3)
+#define CW2015_MASK_ATHD		GENMASK(7, 3)
+#define CW2015_MASK_SOC			GENMASK(12, 0)
+
+/* reset gauge of no valid state of charge could be polled for 40s */
+#define CW2015_BAT_SOC_ERROR_MS		(40 * MSEC_PER_SEC)
+/* reset gauge if state of charge stuck for half an hour during charging */
+#define CW2015_BAT_CHARGING_STUCK_MS	(1800 * MSEC_PER_SEC)
+
+/* poll interval from CellWise GPL Android driver example */
+#define CW2015_DEFAULT_POLL_INTERVAL_MS		8000
+
+#define CW2015_AVERAGING_SAMPLES		3
+
+struct cw_battery {
+	struct device *dev;
+	struct workqueue_struct *battery_workqueue;
+	struct delayed_work battery_delay_work;
+	struct regmap *regmap;
+	struct power_supply *rk_bat;
+	struct power_supply_battery_info battery;
+	u8 *bat_profile;
+
+	bool charger_attached;
+	bool battery_changed;
+
+	int soc;
+	int voltage_mv;
+	int status;
+	int time_to_empty;
+	int charge_count;
+
+	u32 poll_interval_ms;
+	u8 alert_level;
+
+	unsigned int read_errors;
+	unsigned int charge_stuck_cnt;
+};
+
+static int cw_read_word(struct cw_battery *cw_bat, u8 reg, u16 *val)
+{
+	__be16 value;
+	int ret;
+
+	ret = regmap_bulk_read(cw_bat->regmap, reg, &value, sizeof(value));
+	if (ret)
+		return ret;
+
+	*val = be16_to_cpu(value);
+	return 0;
+}
+
+int cw_update_profile(struct cw_battery *cw_bat)
+{
+	int ret, i;
+	unsigned int reg_val;
+	u8 reset_val;
+
+	/* make sure gauge is not in sleep mode */
+	ret = regmap_read(cw_bat->regmap, CW2015_REG_MODE, &reg_val);
+	if (ret)
+		return ret;
+
+	reset_val = reg_val;
+	if ((reg_val & CW2015_MODE_SLEEP_MASK) == CW2015_MODE_SLEEP) {
+		dev_err(cw_bat->dev,
+			"Device is in sleep mode, can't update battery info");
+		return -EINVAL;
+	}
+
+	/* write new battery info */
+	ret = regmap_raw_write(cw_bat->regmap, CW2015_REG_BATINFO,
+				cw_bat->bat_profile,
+				CW2015_SIZE_BATINFO);
+	if (ret)
+		return ret;
+
+	/* set config update flag  */
+	reg_val |= CW2015_CONFIG_UPDATE_FLG;
+	reg_val &= ~CW2015_MASK_ATHD;
+	reg_val |= CW2015_ATHD(cw_bat->alert_level);
+	ret = regmap_write(cw_bat->regmap, CW2015_REG_CONFIG, reg_val);
+	if (ret)
+		return ret;
+
+	/* reset gauge to apply new battery profile */
+	reset_val &= ~CW2015_MODE_RESTART;
+	reg_val = reset_val | CW2015_MODE_RESTART;
+	ret = regmap_write(cw_bat->regmap, CW2015_REG_MODE, reg_val);
+	if (ret)
+		return ret;
+
+	/* wait for gauge to reset */
+	msleep(20);
+
+	/* clear reset flag */
+	ret = regmap_write(cw_bat->regmap, CW2015_REG_MODE, reset_val);
+	if (ret)
+		return ret;
+
+	/* wait for gauge to become ready */
+	for (i = 0; i < CW2015_READ_TRIES; i++) {
+		ret = regmap_read(cw_bat->regmap, CW2015_REG_SOC, &reg_val);
+		if (ret)
+			return ret;
+		/* SoC must not be more than 100% */
+		else if (reg_val <= 100)
+			break;
+
+		msleep(100);
+	}
+
+	if (i >= CW2015_READ_TRIES) {
+		reg_val = CW2015_MODE_SLEEP;
+		regmap_write(cw_bat->regmap, CW2015_REG_MODE, reg_val);
+		dev_err(cw_bat->dev,
+			"Gauge did not become ready after profile upload");
+		return -ETIMEDOUT;
+	}
+
+
+	dev_dbg(cw_bat->dev, "Battery profile updated");
+	return 0;
+}
+
+static int cw_init(struct cw_battery *cw_bat)
+{
+	int ret;
+	unsigned int reg_val = CW2015_MODE_SLEEP;
+
+	if ((reg_val & CW2015_MODE_SLEEP_MASK) == CW2015_MODE_SLEEP) {
+		reg_val = CW2015_MODE_NORMAL;
+		ret = regmap_write(cw_bat->regmap, CW2015_REG_MODE, reg_val);
+		if (ret)
+			return ret;
+	}
+
+	ret = regmap_read(cw_bat->regmap, CW2015_REG_CONFIG, &reg_val);
+	if (ret)
+		return ret;
+
+	if ((reg_val & CW2015_MASK_ATHD) != CW2015_ATHD(cw_bat->alert_level)) {
+		dev_dbg(cw_bat->dev, "Setting new alert level");
+		reg_val &= ~CW2015_MASK_ATHD;
+		reg_val |= ~CW2015_ATHD(cw_bat->alert_level);
+		ret = regmap_write(cw_bat->regmap, CW2015_REG_CONFIG, reg_val);
+		if (ret)
+			return ret;
+	}
+
+	ret = regmap_read(cw_bat->regmap, CW2015_REG_CONFIG, &reg_val);
+	if (ret)
+		return ret;
+
+	if (!(reg_val & CW2015_CONFIG_UPDATE_FLG)) {
+		dev_dbg(cw_bat->dev,
+			"Battery profile not present, uploading battery profile");
+		if (cw_bat->bat_profile) {
+			ret = cw_update_profile(cw_bat);
+			if (ret) {
+				dev_err(cw_bat->dev,
+					 "Failed to upload battery info\n");
+				return ret;
+			}
+		} else {
+			dev_warn(cw_bat->dev,
+				"Have no battery profile for uploading, continuing without profile");
+		}
+	} else if (cw_bat->bat_profile) {
+		u8 bat_info[CW2015_SIZE_BATINFO];
+
+		ret = regmap_raw_read(cw_bat->regmap, CW2015_REG_BATINFO,
+					bat_info, CW2015_SIZE_BATINFO);
+		if (ret) {
+			dev_err(cw_bat->dev,
+				 "Failed to read stored battery profile\n");
+			return ret;
+		}
+
+		if (memcmp(bat_info, cw_bat->bat_profile,
+				CW2015_SIZE_BATINFO)) {
+			dev_warn(cw_bat->dev, "Replacing stored battery profile");
+			ret = cw_update_profile(cw_bat);
+			if (ret)
+				return ret;
+		}
+	} else {
+		dev_warn(cw_bat->dev,
+			"Can't check current battery profile, no profile provided");
+	}
+
+	dev_dbg(cw_bat->dev, "Battery profile configured");
+	return 0;
+}
+
+static int cw_power_on_reset(struct cw_battery *cw_bat)
+{
+	int ret;
+	unsigned char reset_val;
+
+	reset_val = CW2015_MODE_SLEEP;
+	ret = regmap_write(cw_bat->regmap, CW2015_REG_MODE, reset_val);
+	if (ret)
+		return ret;
+
+	/* wait for gauge to enter sleep */
+	msleep(20);
+
+	reset_val = CW2015_MODE_NORMAL;
+	ret = regmap_write(cw_bat->regmap, CW2015_REG_MODE, reset_val);
+	if (ret)
+		return ret;
+
+	ret = cw_init(cw_bat);
+	if (ret)
+		return ret;
+	return 0;
+}
+
+#define HYSTERESIS(current, previous, up, down) \
+	(((current) < (previous) + (up)) && ((current) > (previous) - (down)))
+
+static int cw_get_soc(struct cw_battery *cw_bat)
+{
+	unsigned int soc;
+	int ret;
+
+	ret = regmap_read(cw_bat->regmap, CW2015_REG_SOC, &soc);
+	if (ret)
+		return ret;
+
+	if (soc > 100) {
+		int max_error_cycles = CW2015_BAT_SOC_ERROR_MS /
+					cw_bat->poll_interval_ms;
+
+		dev_err(cw_bat->dev, "Invalid SoC %d%%", soc);
+		cw_bat->read_errors++;
+		if (cw_bat->read_errors > max_error_cycles) {
+			dev_warn(cw_bat->dev,
+				"Too many invalid SoC reports, resetting gauge");
+			cw_power_on_reset(cw_bat);
+			cw_bat->read_errors = 0;
+		}
+		return cw_bat->soc;
+	}
+	cw_bat->read_errors = 0;
+
+	/* Reset gauge if stuck while charging */
+	if (cw_bat->status == POWER_SUPPLY_STATUS_CHARGING &&
+			soc == cw_bat->soc) {
+		int max_stuck_cycles = CW2015_BAT_CHARGING_STUCK_MS /
+					cw_bat->poll_interval_ms;
+
+		cw_bat->charge_stuck_cnt++;
+		if (cw_bat->charge_stuck_cnt > max_stuck_cycles) {
+			dev_warn(cw_bat->dev,
+				"SoC stuck @%u%%, resetting gauge", soc);
+			cw_power_on_reset(cw_bat);
+			cw_bat->charge_stuck_cnt = 0;
+		}
+	} else {
+		cw_bat->charge_stuck_cnt = 0;
+	}
+
+	/* Ignore voltage dips during charge */
+	if (cw_bat->charger_attached &&
+			HYSTERESIS(soc, cw_bat->soc, 0, 3)) {
+		soc = cw_bat->soc;
+	}
+
+	/* Ignore voltage spikes during discharge */
+	if (!cw_bat->charger_attached &&
+			HYSTERESIS(soc, cw_bat->soc, 3, 0)) {
+		soc = cw_bat->soc;
+	}
+
+	return soc;
+}
+
+static int cw_get_voltage(struct cw_battery *cw_bat)
+{
+	int ret, i, voltage_mv;
+	u16 reg_val;
+	u32 avg = 0;
+
+	for (i = 0; i < CW2015_AVERAGING_SAMPLES; i++) {
+		ret = cw_read_word(cw_bat, CW2015_REG_VCELL, &reg_val);
+		if (ret)
+			return ret;
+
+		avg += reg_val;
+	}
+	avg /= CW2015_AVERAGING_SAMPLES;
+
+	/*
+	 * 305 uV per ADC step
+	 * Use 312 / 1024  as efficient approximation of 305 / 1000
+	 * Negligible error of 0.1%
+	 */
+	voltage_mv = avg * 312 / 1024;
+
+	dev_dbg(cw_bat->dev, "Read voltage: %d mV, raw=0x%04x\n",
+		voltage_mv, reg_val);
+	return voltage_mv;
+}
+
+static int cw_get_time_to_empty(struct cw_battery *cw_bat)
+{
+	int ret;
+	u16 value16;
+
+	ret = cw_read_word(cw_bat, CW2015_REG_RRT_ALERT, &value16);
+	if (ret)
+		return ret;
+
+	return value16 & CW2015_MASK_SOC;
+}
+
+static void cw_update_charge_status(struct cw_battery *cw_bat)
+{
+	int ret;
+
+	ret = power_supply_am_i_supplied(cw_bat->rk_bat);
+	if (ret < 0) {
+		dev_warn(cw_bat->dev, "Failed to get supply state: %d",
+				ret);
+	} else {
+		bool charger_attached;
+
+		charger_attached = !!ret;
+		if (cw_bat->charger_attached != charger_attached) {
+			cw_bat->battery_changed = true;
+			if (charger_attached)
+				cw_bat->charge_count++;
+		}
+		cw_bat->charger_attached = charger_attached;
+	}
+}
+
+static void cw_update_soc(struct cw_battery *cw_bat)
+{
+	int soc;
+
+	soc = cw_get_soc(cw_bat);
+	if (soc < 0)
+		dev_err(cw_bat->dev, "Failed to get SoC from gauge: %d",
+			soc);
+	else if (cw_bat->soc != soc) {
+		cw_bat->soc = soc;
+		cw_bat->battery_changed = true;
+	}
+}
+
+static void cw_update_voltage(struct cw_battery *cw_bat)
+{
+	int voltage_mv;
+
+	voltage_mv = cw_get_voltage(cw_bat);
+	if (voltage_mv < 0)
+		dev_err(cw_bat->dev, "Failed to get voltage from gauge: %d",
+			voltage_mv);
+	else
+		cw_bat->voltage_mv = voltage_mv;
+}
+
+static void cw_update_status(struct cw_battery *cw_bat)
+{
+	int status = POWER_SUPPLY_STATUS_DISCHARGING;
+
+	if (cw_bat->charger_attached) {
+		if (cw_bat->soc >= 100)
+			status = POWER_SUPPLY_STATUS_FULL;
+		else
+			status = POWER_SUPPLY_STATUS_CHARGING;
+	}
+
+	if (cw_bat->status != status)
+		cw_bat->battery_changed = true;
+	cw_bat->status = status;
+}
+
+static void cw_update_time_to_empty(struct cw_battery *cw_bat)
+{
+	int time_to_empty;
+
+	time_to_empty = cw_get_time_to_empty(cw_bat);
+	if (time_to_empty < 0)
+		dev_err(cw_bat->dev, "Failed to get time to empty from gauge: %d",
+			time_to_empty);
+	else if (cw_bat->time_to_empty != time_to_empty) {
+		cw_bat->time_to_empty = time_to_empty;
+		cw_bat->battery_changed = true;
+	}
+}
+
+static void cw_bat_work(struct work_struct *work)
+{
+	struct delayed_work *delay_work;
+	struct cw_battery *cw_bat;
+	int ret;
+	unsigned int reg_val;
+	int i = 0;
+
+	delay_work = to_delayed_work(work);
+	cw_bat =
+		container_of(delay_work, struct cw_battery, battery_delay_work);
+	ret = regmap_read(cw_bat->regmap, CW2015_REG_MODE, &reg_val);
+	if (ret) {
+		dev_err(cw_bat->dev, "Failed to read mode from gauge: %d", ret);
+	} else {
+		if ((reg_val & CW2015_MODE_SLEEP_MASK) == CW2015_MODE_SLEEP) {
+			for (i = 0; i < CW2015_RESET_TRIES; i++) {
+				if (cw_power_on_reset(cw_bat) == 0)
+					break;
+			}
+		}
+		cw_update_soc(cw_bat);
+		cw_update_voltage(cw_bat);
+		cw_update_charge_status(cw_bat);
+		cw_update_status(cw_bat);
+		cw_update_time_to_empty(cw_bat);
+	}
+	dev_dbg(cw_bat->dev, "charger_attached = %d", cw_bat->charger_attached);
+	dev_dbg(cw_bat->dev, "status = %d", cw_bat->status);
+	dev_dbg(cw_bat->dev, "soc = %d%%", cw_bat->soc);
+	dev_dbg(cw_bat->dev, "voltage = %dmV", cw_bat->voltage_mv);
+
+	if (cw_bat->battery_changed)
+		power_supply_changed(cw_bat->rk_bat);
+	cw_bat->battery_changed = false;
+
+	queue_delayed_work(cw_bat->battery_workqueue,
+			   &cw_bat->battery_delay_work,
+			   msecs_to_jiffies(cw_bat->poll_interval_ms));
+}
+
+static bool cw_battery_valid_time_to_empty(struct cw_battery *cw_bat)
+{
+	return cw_bat->time_to_empty > 0 &&
+		cw_bat->time_to_empty < CW2015_MASK_SOC &&
+		cw_bat->status == POWER_SUPPLY_STATUS_DISCHARGING;
+}
+
+static int cw_battery_get_property(struct power_supply *psy,
+				   enum power_supply_property psp,
+				   union power_supply_propval *val)
+{
+	int ret = 0;
+	struct cw_battery *cw_bat;
+
+	cw_bat = power_supply_get_drvdata(psy);
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CAPACITY:
+		val->intval = cw_bat->soc;
+		break;
+
+	case POWER_SUPPLY_PROP_STATUS:
+		val->intval = cw_bat->status;
+		break;
+
+	case POWER_SUPPLY_PROP_PRESENT:
+		val->intval = !!cw_bat->voltage_mv;
+		break;
+
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		val->intval = cw_bat->voltage_mv * 1000;
+		break;
+
+	case POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW:
+		if (cw_battery_valid_time_to_empty(cw_bat))
+			val->intval = cw_bat->time_to_empty;
+		else
+			val->intval = 0;
+		break;
+
+	case POWER_SUPPLY_PROP_TECHNOLOGY:
+		val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
+		break;
+
+	case POWER_SUPPLY_PROP_CHARGE_COUNTER:
+		val->intval = cw_bat->charge_count;
+		break;
+
+	case POWER_SUPPLY_PROP_CHARGE_FULL:
+	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
+		if (cw_bat->battery.charge_full_design_uah > 0)
+			val->intval = cw_bat->battery.charge_full_design_uah;
+		else
+			val->intval = 0;
+		break;
+
+	case POWER_SUPPLY_PROP_CURRENT_NOW:
+		if (cw_battery_valid_time_to_empty(cw_bat) &&
+			cw_bat->battery.charge_full_design_uah > 0) {
+			/* calculate remaining capacity */
+			val->intval = cw_bat->battery.charge_full_design_uah;
+			val->intval = val->intval * cw_bat->soc / 100;
+
+			/* estimate current based on time to empty */
+			val->intval = 60 * val->intval / cw_bat->time_to_empty;
+		} else {
+			val->intval = 0;
+		}
+
+		break;
+
+	default:
+		break;
+	}
+	return ret;
+}
+
+static enum power_supply_property cw_battery_properties[] = {
+	POWER_SUPPLY_PROP_CAPACITY,
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW,
+	POWER_SUPPLY_PROP_TECHNOLOGY,
+	POWER_SUPPLY_PROP_CHARGE_COUNTER,
+	POWER_SUPPLY_PROP_CHARGE_FULL,
+	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
+	POWER_SUPPLY_PROP_CURRENT_NOW,
+};
+
+static const struct power_supply_desc cw2015_bat_desc = {
+	.name		= "cw2015-battery",
+	.type		= POWER_SUPPLY_TYPE_BATTERY,
+	.properties	= cw_battery_properties,
+	.num_properties	= ARRAY_SIZE(cw_battery_properties),
+	.get_property	= cw_battery_get_property,
+};
+
+static int cw2015_parse_properties(struct cw_battery *cw_bat)
+{
+	struct device *dev = cw_bat->dev;
+	int length;
+	u32 value;
+	int ret;
+
+	length = device_property_read_u8_array(dev, "cellwise,battery-profile",
+						NULL, 0);
+	if (length) {
+		if (length != CW2015_SIZE_BATINFO) {
+			dev_err(cw_bat->dev, "battery-profile must be %d bytes",
+				CW2015_SIZE_BATINFO);
+			return -EINVAL;
+		}
+
+		cw_bat->bat_profile =
+			devm_kzalloc(dev, CW2015_SIZE_BATINFO, GFP_KERNEL);
+		if (!cw_bat->bat_profile) {
+			dev_err(cw_bat->dev,
+				"Failed to allocate memory for battery config info");
+			return -ENOMEM;
+		}
+
+		ret = device_property_read_u8_array(dev,
+						"cellwise,battery-profile",
+						cw_bat->bat_profile,
+						CW2015_SIZE_BATINFO);
+		if (ret)
+			return ret;
+	} else {
+		dev_warn(cw_bat->dev,
+			"No battery-profile found, rolling with current flash contents");
+	}
+
+	cw_bat->poll_interval_ms = CW2015_DEFAULT_POLL_INTERVAL_MS;
+	ret = device_property_read_u32_array(dev,
+						"cellwise,monitor-interval-ms",
+						&value, 1);
+	if (ret >= 0) {
+		dev_dbg(cw_bat->dev, "Overriding default monitor-interval with %u ms",
+			value);
+		cw_bat->poll_interval_ms = value;
+	}
+
+	return 0;
+}
+
+static const struct regmap_range regmap_ranges_rd_yes[] = {
+	regmap_reg_range(CW2015_REG_VERSION, CW2015_REG_VERSION),
+	regmap_reg_range(CW2015_REG_VCELL, CW2015_REG_CONFIG),
+	regmap_reg_range(CW2015_REG_MODE, CW2015_REG_MODE),
+	regmap_reg_range(CW2015_REG_BATINFO,
+				CW2015_REG_BATINFO + CW2015_SIZE_BATINFO - 1),
+};
+
+static const struct regmap_access_table regmap_rd_table = {
+	.yes_ranges = regmap_ranges_rd_yes,
+	.n_yes_ranges = 4,
+};
+
+static const struct regmap_range regmap_ranges_wr_yes[] = {
+	regmap_reg_range(CW2015_REG_RRT_ALERT, CW2015_REG_CONFIG),
+	regmap_reg_range(CW2015_REG_MODE, CW2015_REG_MODE),
+	regmap_reg_range(CW2015_REG_BATINFO,
+				CW2015_REG_BATINFO + CW2015_SIZE_BATINFO - 1),
+};
+
+static const struct regmap_access_table regmap_wr_table = {
+	.yes_ranges = regmap_ranges_wr_yes,
+	.n_yes_ranges = 3,
+};
+
+static const struct regmap_range regmap_ranges_vol_yes[] = {
+	regmap_reg_range(CW2015_REG_VCELL, CW2015_REG_SOC + 1),
+};
+
+static const struct regmap_access_table regmap_vol_table = {
+	.yes_ranges = regmap_ranges_vol_yes,
+	.n_yes_ranges = 1,
+};
+
+static const struct regmap_config cw2015_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.rd_table = &regmap_rd_table,
+	.wr_table = &regmap_wr_table,
+	.volatile_table = &regmap_vol_table,
+	.max_register = CW2015_REG_BATINFO + CW2015_SIZE_BATINFO - 1,
+};
+
+static int cw_bat_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	int ret;
+	struct cw_battery *cw_bat;
+	struct power_supply_config psy_cfg = { };
+
+	cw_bat = devm_kzalloc(&client->dev, sizeof(*cw_bat), GFP_KERNEL);
+	if (!cw_bat)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, cw_bat);
+	cw_bat->dev = &client->dev;
+	cw_bat->soc = 1;
+
+	ret = cw2015_parse_properties(cw_bat);
+	if (ret) {
+		dev_err(cw_bat->dev, "Failed to parse cw2015 properties");
+		return ret;
+	}
+
+	cw_bat->regmap = devm_regmap_init_i2c(client, &cw2015_regmap_config);
+	if (IS_ERR(cw_bat->regmap)) {
+		dev_err(cw_bat->dev, "Failed to allocate regmap: %ld",
+			PTR_ERR(cw_bat->regmap));
+		return PTR_ERR(cw_bat->regmap);
+	}
+
+	ret = cw_init(cw_bat);
+	if (ret) {
+		dev_err(cw_bat->dev, "Init failed: %d", ret);
+		return ret;
+	}
+
+	psy_cfg.drv_data = cw_bat;
+	psy_cfg.fwnode = dev_fwnode(cw_bat->dev);
+
+	cw_bat->rk_bat = devm_power_supply_register(&client->dev,
+		&cw2015_bat_desc, &psy_cfg);
+	if (IS_ERR(cw_bat->rk_bat)) {
+		dev_err(cw_bat->dev, "Failed to register power supply");
+		return -1;
+	}
+
+	ret = power_supply_get_battery_info(cw_bat->rk_bat, &cw_bat->battery);
+	if (ret) {
+		dev_warn(cw_bat->dev,
+			"No monitored battery, some properties will be missing");
+	}
+
+	cw_bat->battery_workqueue = create_singlethread_workqueue("rk_battery");
+	INIT_DELAYED_WORK(&cw_bat->battery_delay_work, cw_bat_work);
+	queue_delayed_work(cw_bat->battery_workqueue,
+			   &cw_bat->battery_delay_work, msecs_to_jiffies(10));
+	return 0;
+}
+
+static int __maybe_unused cw_bat_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct cw_battery *cw_bat = i2c_get_clientdata(client);
+
+	cancel_delayed_work_sync(&cw_bat->battery_delay_work);
+	return 0;
+}
+
+static int __maybe_unused cw_bat_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct cw_battery *cw_bat = i2c_get_clientdata(client);
+
+	queue_delayed_work(cw_bat->battery_workqueue,
+			   &cw_bat->battery_delay_work, 0);
+	return 0;
+}
+
+SIMPLE_DEV_PM_OPS(cw_bat_pm_ops, cw_bat_suspend, cw_bat_resume);
+
+static int cw_bat_remove(struct i2c_client *client)
+{
+	struct cw_battery *cw_bat = i2c_get_clientdata(client);
+
+	cancel_delayed_work_sync(&cw_bat->battery_delay_work);
+	power_supply_put_battery_info(cw_bat->rk_bat, &cw_bat->battery);
+	return 0;
+}
+
+static const struct i2c_device_id cw_bat_id_table[] = {
+	{ "cw2015", 0 },
+	{ }
+};
+
+static const struct of_device_id cw2015_of_match[] = {
+	{ .compatible = "cellwise,cw2015" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, cw2015_of_match);
+
+static struct i2c_driver cw_bat_driver = {
+	.driver = {
+		.name = "cw2015",
+		.pm = &cw_bat_pm_ops,
+	},
+	.probe = cw_bat_probe,
+	.remove = cw_bat_remove,
+	.id_table = cw_bat_id_table,
+};
+
+module_i2c_driver(cw_bat_driver);
+
+MODULE_AUTHOR("xhc<xhc@rock-chips.com>");
+MODULE_AUTHOR("Tobias Schramm <t.schramm@manjaro.org>");
+MODULE_DESCRIPTION("cw2015/cw2013 battery driver");
+MODULE_LICENSE("GPL");
-- 
2.24.1


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

* Re: [PATCH v3 3/3] power: supply: add CellWise cw2015 fuel gauge driver
  2020-03-11  9:30 ` [PATCH v3 3/3] power: supply: add CellWise cw2015 fuel gauge driver Tobias Schramm
@ 2020-03-11 10:18   ` Andy Shevchenko
  2020-03-11 10:22     ` Andy Shevchenko
  2020-03-11 23:51     ` Tobias Schramm
  0 siblings, 2 replies; 10+ messages in thread
From: Andy Shevchenko @ 2020-03-11 10:18 UTC (permalink / raw)
  To: Tobias Schramm
  Cc: Sebastian Reichel, Rob Herring, Mark Rutland, Maxime Ripard,
	Sam Ravnborg, Heiko Stuebner, Stephan Gerhold, Mark Brown,
	Mauro Carvalho Chehab, David S. Miller, Greg Kroah-Hartman,
	Jonathan Cameron, linux-pm, devicetree, linux-kernel

On Wed, Mar 11, 2020 at 10:30:43AM +0100, Tobias Schramm wrote:
> This patch adds a driver for the CellWise cw2015 fuel gauge.
> 
> The CellWise cw2015 is a shuntless, single-cell Li-Ion fuel gauge used
> in the pine64 Pinebook Pro laptop and some Raspberry Pi UPS HATs.

Thank you for an update!
My comments below.

...

> +	/* wait for gauge to become ready */
> +	for (i = 0; i < CW2015_READ_TRIES; i++) {
> +		ret = regmap_read(cw_bat->regmap, CW2015_REG_SOC, &reg_val);
> +		if (ret)
> +			return ret;
> +		/* SoC must not be more than 100% */
> +		else if (reg_val <= 100)
> +			break;
> +
> +		msleep(100);
> +	}

Have you considered to use regmap_read_poll_timeout()?

> +
> +	if (i >= CW2015_READ_TRIES) {
> +		reg_val = CW2015_MODE_SLEEP;
> +		regmap_write(cw_bat->regmap, CW2015_REG_MODE, reg_val);
> +		dev_err(cw_bat->dev,
> +			"Gauge did not become ready after profile upload");
> +		return -ETIMEDOUT;
> +	}

...

> +		if (memcmp(bat_info, cw_bat->bat_profile,
> +				CW2015_SIZE_BATINFO)) {

I think it's pretty much okay to have this on one line, disregard 80 limit
(it's only 1 extra).

...

> +static int cw_get_soc(struct cw_battery *cw_bat)
> +{
> +	unsigned int soc;
> +	int ret;
> +
> +	ret = regmap_read(cw_bat->regmap, CW2015_REG_SOC, &soc);
> +	if (ret)
> +		return ret;
> +
> +	if (soc > 100) {

> +		int max_error_cycles = CW2015_BAT_SOC_ERROR_MS /
> +					cw_bat->poll_interval_ms;

The following looks better

		int max_error_cycles =
			CW2015_BAT_SOC_ERROR_MS / cw_bat->poll_interval_ms;

Applies to all similar places in the code.

> +		dev_err(cw_bat->dev, "Invalid SoC %d%%", soc);
> +		cw_bat->read_errors++;
> +		if (cw_bat->read_errors > max_error_cycles) {
> +			dev_warn(cw_bat->dev,
> +				"Too many invalid SoC reports, resetting gauge");
> +			cw_power_on_reset(cw_bat);
> +			cw_bat->read_errors = 0;
> +		}
> +		return cw_bat->soc;
> +	}
> +	cw_bat->read_errors = 0;
> +
> +	/* Reset gauge if stuck while charging */

> +	if (cw_bat->status == POWER_SUPPLY_STATUS_CHARGING &&
> +			soc == cw_bat->soc) {

A bit strange indentation, and honestly I would leave it on one line, but it's up to you.

> +		int max_stuck_cycles = CW2015_BAT_CHARGING_STUCK_MS /
> +					cw_bat->poll_interval_ms;
> +
> +		cw_bat->charge_stuck_cnt++;
> +		if (cw_bat->charge_stuck_cnt > max_stuck_cycles) {
> +			dev_warn(cw_bat->dev,
> +				"SoC stuck @%u%%, resetting gauge", soc);
> +			cw_power_on_reset(cw_bat);
> +			cw_bat->charge_stuck_cnt = 0;
> +		}
> +	} else {
> +		cw_bat->charge_stuck_cnt = 0;
> +	}
> +
> +	/* Ignore voltage dips during charge */

> +	if (cw_bat->charger_attached &&
> +			HYSTERESIS(soc, cw_bat->soc, 0, 3)) {

This is pretty much one line (77), check your editor settings and update all
similar places in the code.

> +		soc = cw_bat->soc;
> +	}
> +
> +	/* Ignore voltage spikes during discharge */
> +	if (!cw_bat->charger_attached &&
> +			HYSTERESIS(soc, cw_bat->soc, 3, 0)) {
> +		soc = cw_bat->soc;
> +	}
> +
> +	return soc;
> +}

...

> +	cw_bat =
> +		container_of(delay_work, struct cw_battery, battery_delay_work);

It will be better to read if it would be one line.

...

> +static bool cw_battery_valid_time_to_empty(struct cw_battery *cw_bat)
> +{
> +	return cw_bat->time_to_empty > 0 &&
> +		cw_bat->time_to_empty < CW2015_MASK_SOC &&
> +		cw_bat->status == POWER_SUPPLY_STATUS_DISCHARGING;

Fix indentation to be all 'c':s in one column.

> +}

...

> +static int cw2015_parse_properties(struct cw_battery *cw_bat)
> +{
> +	struct device *dev = cw_bat->dev;
> +	int length;
> +	u32 value;
> +	int ret;
> +

> +	length = device_property_read_u8_array(dev, "cellwise,battery-profile",
> +						NULL, 0);

device_property_count_u8();

> +	if (length) {
> +		if (length != CW2015_SIZE_BATINFO) {
> +			dev_err(cw_bat->dev, "battery-profile must be %d bytes",
> +				CW2015_SIZE_BATINFO);
> +			return -EINVAL;
> +		}
> +
> +		cw_bat->bat_profile =
> +			devm_kzalloc(dev, CW2015_SIZE_BATINFO, GFP_KERNEL);

Replace with length (so, you will have one point of validation), and put on
one line.

> +		if (!cw_bat->bat_profile) {
> +			dev_err(cw_bat->dev,
> +				"Failed to allocate memory for battery config info");
> +			return -ENOMEM;
> +		}
> +
> +		ret = device_property_read_u8_array(dev,
> +						"cellwise,battery-profile",
> +						cw_bat->bat_profile,

> +						CW2015_SIZE_BATINFO);

length.

> +		if (ret)
> +			return ret;
> +	} else {
> +		dev_warn(cw_bat->dev,
> +			"No battery-profile found, rolling with current flash contents");
> +	}
> +
> +	cw_bat->poll_interval_ms = CW2015_DEFAULT_POLL_INTERVAL_MS;

> +	ret = device_property_read_u32_array(dev,
> +						"cellwise,monitor-interval-ms",

It's fine to have it on one line.

> +						&value, 1);
> +	if (ret >= 0) {
> +		dev_dbg(cw_bat->dev, "Overriding default monitor-interval with %u ms",
> +			value);
> +		cw_bat->poll_interval_ms = value;
> +	}
> +
> +	return 0;
> +}

...

> +	regmap_reg_range(CW2015_REG_BATINFO,
> +				CW2015_REG_BATINFO + CW2015_SIZE_BATINFO - 1),

Indentation issue. Check all similar places.

...

> +	cw_bat->rk_bat = devm_power_supply_register(&client->dev,
> +		&cw2015_bat_desc, &psy_cfg);
> +	if (IS_ERR(cw_bat->rk_bat)) {
> +		dev_err(cw_bat->dev, "Failed to register power supply");

> +		return -1;

Do not shadow an error code.

> +	}

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH v3 3/3] power: supply: add CellWise cw2015 fuel gauge driver
  2020-03-11 10:18   ` Andy Shevchenko
@ 2020-03-11 10:22     ` Andy Shevchenko
  2020-03-11 23:51     ` Tobias Schramm
  1 sibling, 0 replies; 10+ messages in thread
From: Andy Shevchenko @ 2020-03-11 10:22 UTC (permalink / raw)
  To: Tobias Schramm
  Cc: Sebastian Reichel, Rob Herring, Mark Rutland, Maxime Ripard,
	Sam Ravnborg, Heiko Stuebner, Stephan Gerhold, Mark Brown,
	Mauro Carvalho Chehab, David S. Miller, Greg Kroah-Hartman,
	Jonathan Cameron, linux-pm, devicetree, linux-kernel

On Wed, Mar 11, 2020 at 12:18:30PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 11, 2020 at 10:30:43AM +0100, Tobias Schramm wrote:

...

> > +	ret = device_property_read_u32_array(dev,
> > +						"cellwise,monitor-interval-ms",
> 
> It's fine to have it on one line.
> 
> > +						&value, 1);


Actually this is simple
	ret = device_property_read_u32(dev, "cellwise,monitor-interval-ms", &value);
(You can use one or two lines on your choice)

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH v3 2/3] dt-bindings: power: supply: add cw2015_battery bindings
  2020-03-11  9:30 ` [PATCH v3 2/3] dt-bindings: power: supply: add cw2015_battery bindings Tobias Schramm
@ 2020-03-11 17:20   ` Daniel Thompson
  2020-03-11 23:17     ` Tobias Schramm
  0 siblings, 1 reply; 10+ messages in thread
From: Daniel Thompson @ 2020-03-11 17:20 UTC (permalink / raw)
  To: Tobias Schramm
  Cc: Andy Shevchenko, Sebastian Reichel, Rob Herring, Mark Rutland,
	Maxime Ripard, Sam Ravnborg, Heiko Stuebner, Stephan Gerhold,
	Mark Brown, Mauro Carvalho Chehab, David S. Miller,
	Greg Kroah-Hartman, Jonathan Cameron, linux-pm, devicetree,
	linux-kernel

On Wed, Mar 11, 2020 at 10:30:42AM +0100, Tobias Schramm wrote:
> This patch adds the dts binding schema for the cw2015 fuel gauge.
> 
> Signed-off-by: Tobias Schramm <t.schramm@manjaro.org>
> ---
>  .../bindings/power/supply/cw2015_battery.yaml | 83 +++++++++++++++++++
>  1 file changed, 83 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/supply/cw2015_battery.yaml
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/cw2015_battery.yaml b/Documentation/devicetree/bindings/power/supply/cw2015_battery.yaml
> new file mode 100644
> index 000000000000..647dbc6e136e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/cw2015_battery.yaml
> @@ -0,0 +1,83 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/supply/cw2015_battery.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Battery driver for CW2015 shuntless fule gauge by CellWise.

s/fule/fuel/


> +
> +maintainers:
> +  - Tobias Schramm <t.schramm@manjaro.org>
> +
> +description: |
> +  The driver can utilize information from a simple-battery linked via a
> +  phandle in monitored-battery. If specified the driver uses the
> +  charge-full-design-microamp-hours property of the battery.
> +
> +properties:
> +  compatible:
> +    const: cellwise,cw2015
> +
> +  reg:
> +    items:
> +      - description: i2c address
> +
> +  cellwise,battery-profile:
> +    description: |
> +      This property specifies characteristics of the battery used. The format
> +      of this binary blob is kept secret by CellWise. The only way to obtain
> +      it is to mail two batteries to a test facility of CellWise and receive
> +      back a test report with the binary blob.
> +    allOf:
> +      - $ref: /schemas/types.yaml#definitions/uint8-array
> +    items:
> +      - minItems: 64
> +        maxItems: 64
> +
> +  cellwise,monitor-interval-ms:
> +    description:
> +      Specifies the interval in milliseconds gauge values are polled at
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  power-supplies:
> +    description:
> +      Specifies supplies used for charging the battery connected to this gauge
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/phandle-array
> +      - minItems: 1
> +        maxItems: 8 # Should be enough

Is it necessary to set a maximum? power_supply.txt is still a text file
but there is no mention of a maximum there.


Daniel.

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

* Re: [PATCH v3 2/3] dt-bindings: power: supply: add cw2015_battery bindings
  2020-03-11 17:20   ` Daniel Thompson
@ 2020-03-11 23:17     ` Tobias Schramm
  2020-03-12 10:17       ` Daniel Thompson
  0 siblings, 1 reply; 10+ messages in thread
From: Tobias Schramm @ 2020-03-11 23:17 UTC (permalink / raw)
  To: Daniel Thompson, Tobias Schramm
  Cc: Andy Shevchenko, Sebastian Reichel, Rob Herring, Mark Rutland,
	Maxime Ripard, Sam Ravnborg, Heiko Stuebner, Stephan Gerhold,
	Mark Brown, Mauro Carvalho Chehab, David S. Miller,
	Greg Kroah-Hartman, Jonathan Cameron, linux-pm, devicetree,
	linux-kernel

Hi Daniel,

thanks for reviewing. The typo will be fixed for v4.

>> +  power-supplies:
>> +    description:
>> +      Specifies supplies used for charging the battery connected to this gauge
>> +    allOf:
>> +      - $ref: /schemas/types.yaml#/definitions/phandle-array
>> +      - minItems: 1
>> +        maxItems: 8 # Should be enough
> 
> Is it necessary to set a maximum? power_supply.txt is still a text file
> but there is no mention of a maximum there.
> 
I think so? Removing maxItems and running dtbs_check on a dts with more
than one supply phandle in the power-supplies property results in an error:
linux/arch/arm64/boot/dts/rockchip/rk3399-pinebook-pro.dt.yaml:
cw2015@62: power-supplies: [[142], [50]] is too long

Best Regards,

Tobias

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

* Re: [PATCH v3 3/3] power: supply: add CellWise cw2015 fuel gauge driver
  2020-03-11 10:18   ` Andy Shevchenko
  2020-03-11 10:22     ` Andy Shevchenko
@ 2020-03-11 23:51     ` Tobias Schramm
  1 sibling, 0 replies; 10+ messages in thread
From: Tobias Schramm @ 2020-03-11 23:51 UTC (permalink / raw)
  To: Andy Shevchenko, Tobias Schramm
  Cc: Sebastian Reichel, Rob Herring, Mark Rutland, Maxime Ripard,
	Sam Ravnborg, Heiko Stuebner, Stephan Gerhold, Mark Brown,
	Mauro Carvalho Chehab, David S. Miller, Greg Kroah-Hartman,
	Jonathan Cameron, linux-pm, devicetree, linux-kernel

Hi Andy,

thanks for reviewing again.

>> +	/* wait for gauge to become ready */
>> +	for (i = 0; i < CW2015_READ_TRIES; i++) {
>> +		ret = regmap_read(cw_bat->regmap, CW2015_REG_SOC, &reg_val);
>> +		if (ret)
>> +			return ret;
>> +		/* SoC must not be more than 100% */
>> +		else if (reg_val <= 100)
>> +			break;
>> +
>> +		msleep(100);
>> +	}
> 
> Have you considered to use regmap_read_poll_timeout()?

Neat! That is a much cleaner solution. Will use that in v4.

> 
>> +
>> +	if (i >= CW2015_READ_TRIES) {
>> +		reg_val = CW2015_MODE_SLEEP;
>> +		regmap_write(cw_bat->regmap, CW2015_REG_MODE, reg_val);
>> +		dev_err(cw_bat->dev,
>> +			"Gauge did not become ready after profile upload");
>> +		return -ETIMEDOUT;
>> +	}
> 
> ...
> 
>> +		if (memcmp(bat_info, cw_bat->bat_profile,
>> +				CW2015_SIZE_BATINFO)) {
> 
> I think it's pretty much okay to have this on one line, disregard 80 limit
> (it's only 1 extra).

Ok, will probably do that in a few places.


Best Regards,

Tobias

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

* Re: [PATCH v3 2/3] dt-bindings: power: supply: add cw2015_battery bindings
  2020-03-11 23:17     ` Tobias Schramm
@ 2020-03-12 10:17       ` Daniel Thompson
  0 siblings, 0 replies; 10+ messages in thread
From: Daniel Thompson @ 2020-03-12 10:17 UTC (permalink / raw)
  To: Tobias Schramm
  Cc: Andy Shevchenko, Sebastian Reichel, Rob Herring, Mark Rutland,
	Maxime Ripard, Sam Ravnborg, Heiko Stuebner, Stephan Gerhold,
	Mark Brown, Mauro Carvalho Chehab, David S. Miller,
	Greg Kroah-Hartman, Jonathan Cameron, linux-pm, devicetree,
	linux-kernel

On Thu, Mar 12, 2020 at 12:17:55AM +0100, Tobias Schramm wrote:
> Hi Daniel,
> 
> thanks for reviewing. The typo will be fixed for v4.
> 
> >> +  power-supplies:
> >> +    description:
> >> +      Specifies supplies used for charging the battery connected to this gauge
> >> +    allOf:
> >> +      - $ref: /schemas/types.yaml#/definitions/phandle-array
> >> +      - minItems: 1
> >> +        maxItems: 8 # Should be enough
> > 
> > Is it necessary to set a maximum? power_supply.txt is still a text file
> > but there is no mention of a maximum there.
> > 
> I think so? Removing maxItems and running dtbs_check on a dts with more
> than one supply phandle in the power-supplies property results in an error:
> linux/arch/arm64/boot/dts/rockchip/rk3399-pinebook-pro.dt.yaml:
> cw2015@62: power-supplies: [[142], [50]] is too long

Interesting. I saw the "Should be enough" comment replicated in several
YAML bindings (with varying degress of paranoia about how much "enough" is).
There are also several that simply set minItems without setting
maxItems, perhaps they have just never been any DTs that test those
bindings with more than one item.


Daniel.

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

end of thread, other threads:[~2020-03-12 10:17 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-11  9:30 [PATCH v3 0/3] Add support for CellWise cw2015 fuel gauge Tobias Schramm
2020-03-11  9:30 ` [PATCH v3 1/3] dt-bindings: Document cellwise vendor-prefix Tobias Schramm
2020-03-11  9:30 ` [PATCH v3 2/3] dt-bindings: power: supply: add cw2015_battery bindings Tobias Schramm
2020-03-11 17:20   ` Daniel Thompson
2020-03-11 23:17     ` Tobias Schramm
2020-03-12 10:17       ` Daniel Thompson
2020-03-11  9:30 ` [PATCH v3 3/3] power: supply: add CellWise cw2015 fuel gauge driver Tobias Schramm
2020-03-11 10:18   ` Andy Shevchenko
2020-03-11 10:22     ` Andy Shevchenko
2020-03-11 23:51     ` Tobias Schramm

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).