[PATCH 0/9] Summit SMB3xx driver & device-tree

[PATCH 0/9] Summit SMB3xx driver & device-tree

[David Heidelberg]

We gathered existing patches, fixed and improved what we could and
final result is an working charging driver with device-tree support for
Nexus 7.

At this moment charging works with:
 - Nexus 7 2012 (grouper and tilapia)
 - Nexus 7 2013 (flo and deb)
 - ... and there is more devices equiped with these chargers.

David Heidelberg (7):
  power: supply: smb347-charger: Add delay before getting IRQSTAT
  power: supply: smb347-charger: Use resource-managed API
  dt-bindings: power: supply: Add device-tree binding for Summit SMB3xx
  power: supply: smb347-charger: Implement device-tree support
  power: supply: smb347-charger: Support SMB345 and SMB358
  power: supply: smb347-charger: Remove virtual smb347-battery
  arm: dts: qcom: apq8064-nexus7: Add smb345 charger node

Dmitry Osipenko (2):
  power: supply: smb347-charger: IRQSTAT_D is volatile
  power: supply: smb347-charger: Replace mutex with IRQ disable/enable

 .../power/supply/summit,smb347-charger.yaml   | 224 ++++++++
 .../boot/dts/qcom-apq8064-asus-nexus7-flo.dts |  22 +-
 drivers/power/supply/Kconfig                  |   6 +-
 drivers/power/supply/smb347-charger.c         | 510 +++++++++---------
 .../dt-bindings/power/summit,smb347-charger.h |  19 +
 5 files changed, 526 insertions(+), 255 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/supply/summit,smb347-charger.yaml
 create mode 100644 include/dt-bindings/power/summit,smb347-charger.h

-- 
2.25.0

[PATCH 1/9] power: supply: smb347-charger: IRQSTAT_D is volatile

[David Heidelberg]

From: Dmitry Osipenko <digetx@gmail.com>

Fix failure when USB cable is connected:
smb347 2-006a: reading IRQSTAT_D failed

Fixes: 1502cfe19bac ("smb347-charger: Fix battery status reporting logic for charger faults")

Tested-by: David Heidelberg <david@ixit.cz>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: David Heidelberg <david@ixit.cz>
---
 drivers/power/supply/smb347-charger.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/power/supply/smb347-charger.c b/drivers/power/supply/smb347-charger.c
index c1d124b8be0c..d102921b3ab2 100644
--- a/drivers/power/supply/smb347-charger.c
+++ b/drivers/power/supply/smb347-charger.c
@@ -1138,6 +1138,7 @@ static bool smb347_volatile_reg(struct device *dev, unsigned int reg)
 	switch (reg) {
 	case IRQSTAT_A:
 	case IRQSTAT_C:
+	case IRQSTAT_D:
 	case IRQSTAT_E:
 	case IRQSTAT_F:
 	case STAT_A:
-- 
2.25.0

[PATCH 2/9] power: supply: smb347-charger: Add delay before getting IRQSTAT

[David Heidelberg]

This delay-fix is picked up from downstream driver,
we measured that 25 - 35 ms delay ensure that we get required data.

Tested on SMB347 on Nexus 7 2012. Otherwise IRQSTAT_E fails to provide
correct information.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: David Heidelberg <david@ixit.cz>
---
 drivers/power/supply/smb347-charger.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/power/supply/smb347-charger.c b/drivers/power/supply/smb347-charger.c
index d102921b3ab2..f99026d81f2a 100644
--- a/drivers/power/supply/smb347-charger.c
+++ b/drivers/power/supply/smb347-charger.c
@@ -8,6 +8,7 @@
  *          Mika Westerberg <mika.westerberg@linux.intel.com>
  */
 
+#include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/gpio.h>
 #include <linux/kernel.h>
@@ -708,6 +709,9 @@ static irqreturn_t smb347_interrupt(int irq, void *data)
 	bool handled = false;
 	int ret;
 
+	/* SMB347 it needs at least 20ms for setting IRQSTAT_E_*IN_UV_IRQ */
+	usleep_range(25000, 35000);
+
 	ret = regmap_read(smb->regmap, STAT_C, &stat_c);
 	if (ret < 0) {
 		dev_warn(smb->dev, "reading STAT_C failed\n");
-- 
2.25.0

[PATCH 3/9] power: supply: smb347-charger: Use resource-managed API

[David Heidelberg]

Simplify code, more convenient to use with Device Tree.

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: David Heidelberg <david@ixit.cz>
---
 drivers/power/supply/smb347-charger.c | 45 +++++++++++++--------------
 1 file changed, 21 insertions(+), 24 deletions(-)

diff --git a/drivers/power/supply/smb347-charger.c b/drivers/power/supply/smb347-charger.c
index f99026d81f2a..4add9f64ba90 100644
--- a/drivers/power/supply/smb347-charger.c
+++ b/drivers/power/supply/smb347-charger.c
@@ -836,21 +836,31 @@ static int smb347_irq_init(struct smb347_charger *smb,
 			   struct i2c_client *client)
 {
 	const struct smb347_charger_platform_data *pdata = smb->pdata;
-	int ret, irq = gpio_to_irq(pdata->irq_gpio);
+	unsigned long irqflags = IRQF_ONESHOT;
+	int ret;
 
-	ret = gpio_request_one(pdata->irq_gpio, GPIOF_IN, client->name);
-	if (ret < 0)
-		goto fail;
+	/* Requesting GPIO for IRQ is only needed in non-DT way */
+	if (!client->irq) {
+		int irq = gpio_to_irq(pdata->irq_gpio);
+
+		ret = devm_gpio_request_one(smb->dev, pdata->irq_gpio,
+					    GPIOF_IN, client->name);
+		if (ret < 0)
+			return ret;
+
+		irqflags |= IRQF_TRIGGER_FALLING;
+		client->irq = irq;
+	}
 
-	ret = request_threaded_irq(irq, NULL, smb347_interrupt,
-				   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
-				   client->name, smb);
+	ret = devm_request_threaded_irq(smb->dev, client->irq, NULL,
+					smb347_interrupt, irqflags,
+					client->name, smb);
 	if (ret < 0)
-		goto fail_gpio;
+		return ret;
 
 	ret = smb347_set_writable(smb, true);
 	if (ret < 0)
-		goto fail_irq;
+		return ret;
 
 	/*
 	 * Configure the STAT output to be suitable for interrupts: disable
@@ -860,20 +870,10 @@ static int smb347_irq_init(struct smb347_charger *smb,
 				 CFG_STAT_ACTIVE_HIGH | CFG_STAT_DISABLED,
 				 CFG_STAT_DISABLED);
 	if (ret < 0)
-		goto fail_readonly;
+		client->irq = 0;
 
 	smb347_set_writable(smb, false);
-	client->irq = irq;
-	return 0;
 
-fail_readonly:
-	smb347_set_writable(smb, false);
-fail_irq:
-	free_irq(irq, smb);
-fail_gpio:
-	gpio_free(pdata->irq_gpio);
-fail:
-	client->irq = 0;
 	return ret;
 }
 
@@ -1299,11 +1299,8 @@ static int smb347_remove(struct i2c_client *client)
 {
 	struct smb347_charger *smb = i2c_get_clientdata(client);
 
-	if (client->irq) {
+	if (client->irq)
 		smb347_irq_disable(smb);
-		free_irq(client->irq, smb);
-		gpio_free(smb->pdata->irq_gpio);
-	}
 
 	power_supply_unregister(smb->battery);
 	if (smb->pdata->use_usb)
-- 
2.25.0

[PATCH 4/9] dt-bindings: power: supply: Add device-tree binding for Summit SMB3xx

[David Heidelberg]

Summit SMB3xx series is a Programmable Switching Li+ Battery Charger.
This device-tree binding documents Summit SMB345, SMB347 and SMB358 chargers.

Signed-off-by: David Heidelberg <david@ixit.cz>
---
 .../power/supply/summit,smb347-charger.yaml   | 224 ++++++++++++++++++
 .../dt-bindings/power/summit,smb347-charger.h |  19 ++
 2 files changed, 243 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/summit,smb347-charger.yaml
 create mode 100644 include/dt-bindings/power/summit,smb347-charger.h

diff --git a/Documentation/devicetree/bindings/power/supply/summit,smb347-charger.yaml b/Documentation/devicetree/bindings/power/supply/summit,smb347-charger.yaml
new file mode 100644
index 000000000000..1d6bccdcd233
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/summit,smb347-charger.yaml
@@ -0,0 +1,224 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/power/supply/summit,smb347-charger.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Battery charger driver for SMB345, SMB347 and SMB358
+
+maintainers:
+  - David Heidelberg <david@ixit.cz>
+
+properties:
+  compatible:
+    enum:
+      - summit,smb345
+      - summit,smb347
+      - summit,smb358
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  summit,enable-usb-charging:
+    type: boolean
+    description: Enable charging trough USB.
+
+  summit,enable-otg-charging:
+    type: boolean
+    description: Provide power for USB OTG
+
+  summit,enable-mains-charging:
+    type: boolean
+    description: Enable charging trough mains
+
+  summit,enable-chg-ctrl:
+    description: Enable charging control
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+      - enum:
+          - 0 # SMB3XX_CHG_ENABLE_SW SW (I2C interface)
+          - 1 # SMB3XX_CHG_ENABLE_PIN_ACTIVE_LOW Pin control (Active Low)
+          - 2 # SMB3XX_CHG_ENABLE_PIN_ACTIVE_HIGH Pin control (Active High)
+
+  summit,max-chg-curr:
+    description: Maximum current for charging (in uA)
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+
+  summit,max-chg-volt:
+    description: Maximum voltage for charging (in uV)
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 3500000
+    maximum: 4500000
+
+  summit,pre-chg-curr:
+    description: Pre-charging current for charging (in uA)
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+
+  summit,term-curr:
+    description: Charging cycle termination current (in uA)
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+
+  summit,fast-volt-threshold:
+    description: Voltage threshold to transit to fast charge mode (in uV)
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 2400000
+    maximum: 3000000
+
+  summit,mains-curr-limit:
+    description: Maximum input current from AC/DC input (in uA)
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+
+  summit,usb-curr-limit:
+    description: Maximum input current from USB input (in uA)
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+
+  summit,chg-curr-comp:
+    description: Charge current compensation (in uA)
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+
+  summit,chip-temp-threshold:
+    description: Chip temperature for thermal regulation in °C.
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [100, 110, 120, 130]
+
+  summit,soft-cold-temp-limit:
+    description: Cold battery temperature in °C for soft alarm.
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 5, 10, 15]
+
+  summit,soft-hot-temp-limit:
+    description: Hot battery temperature in °C for soft alarm.
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [40, 45, 50, 55]
+
+  summit,hard-cold-temp-limit:
+    description: Cold battery temperature in °C for hard alarm.
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/int32
+    enum: [-5, 0, 5, 10]
+
+  summit,hard-hot-temp-limit:
+    description: Hot battery temperature in °C for hard alarm.
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [50, 55, 60, 65]
+
+  summit,soft-comp-method:
+    description: Soft temperature limit compensation method
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+      - enum:
+          - 0 # SMB3XX_SOFT_TEMP_COMPENSATE_NONE Compensation none
+          - 1 # SMB3XX_SOFT_TEMP_COMPENSATE_CURRENT Current compensation
+          - 2 # SMB3XX_SOFT_TEMP_COMPENSATE_VOLTAGE Voltage compensation
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          enum:
+            - summit,smb345
+            - summit,smb358
+
+    then:
+      properties:
+        summit,max-chg-curr:
+          enum: [ 200000,  450000,  600000,  900000,
+                 1300000, 1500000, 1800000, 2000000]
+
+        summit,pre-chg-curr:
+          enum: [150000, 250000, 350000, 450000]
+
+        summit,term-curr:
+          enum: [ 30000,  40000,  60000,  80000,
+                 100000, 125000, 150000, 200000]
+
+        summit,mains-curr-limit:
+          enum: [ 300000,  500000,  700000, 1000000,
+                 1500000, 1800000, 2000000]
+
+        summit,usb-curr-limit:
+          enum: [ 300000,  500000,  700000, 1000000,
+                 1500000, 1800000, 2000000]
+
+        summit,chg-curr-comp:
+          enum: [200000, 450000, 600000, 900000]
+
+    else:
+      properties:
+        summit,max-chg-curr:
+          enum: [ 700000,  900000, 1200000, 1500000,
+                 1800000, 2000000, 2200000, 2500000]
+
+        summit,pre-chg-curr:
+          enum: [100000, 150000, 200000, 250000]
+
+        summit,term-curr:
+          enum: [ 37500,  50000, 100000, 150000,
+                 200000, 250000, 500000, 600000]
+
+        summit,mains-curr-limit:
+          enum: [ 300000,  500000,  700000,  900000, 1200000,
+                 1500000, 1800000, 2000000, 2200000, 2500000]
+
+        summit,usb-curr-limit:
+          enum: [ 300000,  500000,  700000,  900000, 1200000,
+                 1500000, 1800000, 2000000, 2200000, 2500000]
+
+        summit,chg-curr-comp:
+          enum: [250000, 700000, 900000, 1200000]
+
+required:
+  - compatible
+  - reg
+
+anyOf:
+  - required:
+      - summit,enable-usb-charging
+  - required:
+      - summit,enable-otg-charging
+  - required:
+      - summit,enable-mains-charging
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/power/summit,smb347-charger.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        charger@7f {
+            compatible = "summit,smb347";
+            reg = <0x7f>;
+            status = "okay";
+
+            summit,max-chg-curr = <1800000>;
+            summit,mains-curr-limit = <2000000>;
+            summit,usb-curr-limit = <500000>;
+
+            summit,chip-temp-threshold = <110>;
+            summit,soft-cold-temp-limit = <5>;
+
+            summit,enable-usb-charging;
+            summit,enable-mains-charging;
+
+            summit,enable-chg-ctrl = <SMB3XX_CHG_ENABLE_PIN_ACTIVE_HIGH>;
+        };
+    };
diff --git a/include/dt-bindings/power/summit,smb347-charger.h b/include/dt-bindings/power/summit,smb347-charger.h
new file mode 100644
index 000000000000..d918bf321a71
--- /dev/null
+++ b/include/dt-bindings/power/summit,smb347-charger.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: (GPL-2.0-or-later or MIT) */
+/*
+ * Author: David Heidelberg <david@ixit.cz>
+ */
+
+#ifndef _DT_BINDINGS_SMB347_CHARGER_H
+#define _DT_BINDINGS_SMB347_CHARGER_H
+
+/* Charging compensation method */
+#define SMB3XX_SOFT_TEMP_COMPENSATE_NONE	0
+#define SMB3XX_SOFT_TEMP_COMPENSATE_CURRENT	1
+#define SMB3XX_SOFT_TEMP_COMPENSATE_VOLTAGE	2
+
+/* Charging enable control */
+#define SMB3XX_CHG_ENABLE_SW			0
+#define SMB3XX_CHG_ENABLE_PIN_ACTIVE_LOW	1
+#define SMB3XX_CHG_ENABLE_PIN_ACTIVE_HIGH	2
+
+#endif
-- 
2.25.0

[PATCH 5/9] power: supply: smb347-charger: Implement device-tree support

[David Heidelberg]

This patch makes smb347 charger driver to support dt binding. All legacy
platform data now can be parsed from dt.
Because of that smb347 is i2c client driver, IRQ number can be passed
automatically through client's irq variable if it is defined in dt.
No more to use requesting gpio to irq manually in dt-way.

Based on: https://patchwork.kernel.org/patch/4284731/
Original author: Jonghwa Lee <jonghwa3.lee@samsung.com>

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: David Heidelberg <david@ixit.cz>
---
 drivers/power/supply/smb347-charger.c | 107 +++++++++++++++++++++++---
 1 file changed, 97 insertions(+), 10 deletions(-)

diff --git a/drivers/power/supply/smb347-charger.c b/drivers/power/supply/smb347-charger.c
index 4add9f64ba90..852d2ab566e0 100644
--- a/drivers/power/supply/smb347-charger.c
+++ b/drivers/power/supply/smb347-charger.c
@@ -1180,6 +1180,87 @@ static bool smb347_readable_reg(struct device *dev, unsigned int reg)
 	return smb347_volatile_reg(dev, reg);
 }
 
+static void smb347_dt_parse_pdata(struct device_node *np,
+				  struct smb347_charger_platform_data *pdata)
+{
+	/* Charging constraints */
+	of_property_read_u32(np, "summit,max-chg-curr",
+			     &pdata->max_charge_current);
+	of_property_read_u32(np, "summit,max-chg-volt",
+			     &pdata->max_charge_voltage);
+	of_property_read_u32(np, "summit,pre-chg-curr",
+			     &pdata->pre_charge_current);
+	of_property_read_u32(np, "summit,term-curr",
+			     &pdata->termination_current);
+	of_property_read_u32(np, "summit,fast-volt-threshold",
+			     &pdata->pre_to_fast_voltage);
+	of_property_read_u32(np, "summit,mains-curr-limit",
+			     &pdata->mains_current_limit);
+	of_property_read_u32(np, "summit,usb-curr-limit",
+			     &pdata->usb_hc_current_limit);
+
+	/* For thermometer monitoring */
+	of_property_read_u32(np, "summit,chip-temp-threshold",
+			     &pdata->chip_temp_threshold);
+	if (of_property_read_u32(np, "summit,soft-cold-temp-limit",
+				 &pdata->soft_cold_temp_limit))
+		pdata->soft_cold_temp_limit = SMB347_TEMP_USE_DEFAULT;
+	if (of_property_read_u32(np, "summit,soft-hot-temp-limit",
+				 &pdata->soft_hot_temp_limit))
+		pdata->soft_hot_temp_limit = SMB347_TEMP_USE_DEFAULT;
+	if (of_property_read_u32(np, "summit,hard-cold-temp-limit",
+				 &pdata->hard_cold_temp_limit))
+		pdata->hard_cold_temp_limit = SMB347_TEMP_USE_DEFAULT;
+	if (of_property_read_u32(np, "summit,hard-hot-temp-limit",
+				 &pdata->hard_hot_temp_limit))
+		pdata->hard_hot_temp_limit = SMB347_TEMP_USE_DEFAULT;
+
+	/* Suspend when battery temperature is outside hard limits */
+	if (pdata->hard_cold_temp_limit != SMB347_TEMP_USE_DEFAULT ||
+	    pdata->hard_hot_temp_limit != SMB347_TEMP_USE_DEFAULT)
+		pdata->suspend_on_hard_temp_limit = true;
+
+	if (of_property_read_u32(np, "summit,soft-comp-method",
+				 &pdata->soft_temp_limit_compensation))
+		pdata->soft_temp_limit_compensation =
+				SMB347_SOFT_TEMP_COMPENSATE_DEFAULT;
+
+	of_property_read_u32(np, "summit,chg-curr-comp",
+			     &pdata->charge_current_compensation);
+
+	/* Supported charging mode */
+	pdata->use_mains =
+		of_property_read_bool(np, "summit,enable-mains-charging");
+	pdata->use_usb =
+		of_property_read_bool(np, "summit,enable-usb-charging");
+	pdata->use_usb_otg =
+		of_property_read_bool(np, "summit,enable-otg-charging");
+
+	/* Enable charging method */
+	of_property_read_u32(np, "summit,enable-chg-ctrl",
+			     &pdata->enable_control);
+
+	/* If IRQ is enabled or not */
+	if (!of_find_property(np, "interrupts", NULL))
+		pdata->irq_gpio = -1;
+}
+
+static struct smb347_charger_platform_data
+			*smb347_get_platdata(struct device *dev)
+{
+	struct smb347_charger_platform_data *pdata;
+
+	if (dev->of_node) {
+		pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+		if (pdata)
+			smb347_dt_parse_pdata(dev->of_node, pdata);
+	} else {
+		pdata = dev_get_platdata(dev);
+	}
+
+	return pdata;
+}
+
 static const struct regmap_config smb347_regmap = {
 	.reg_bits	= 8,
 	.val_bits	= 8,
@@ -1216,28 +1297,26 @@ static int smb347_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
 	static char *battery[] = { "smb347-battery" };
-	const struct smb347_charger_platform_data *pdata;
 	struct power_supply_config mains_usb_cfg = {}, battery_cfg = {};
 	struct device *dev = &client->dev;
 	struct smb347_charger *smb;
 	int ret;
 
-	pdata = dev->platform_data;
-	if (!pdata)
-		return -EINVAL;
-
-	if (!pdata->use_mains && !pdata->use_usb)
-		return -EINVAL;
-
 	smb = devm_kzalloc(dev, sizeof(*smb), GFP_KERNEL);
 	if (!smb)
 		return -ENOMEM;
 
+	smb->pdata = smb347_get_platdata(dev);
+	if (!smb->pdata)
+		return -ENODEV;
+
+	if (!smb->pdata->use_mains && !smb->pdata->use_usb)
+		return -EINVAL;
+
 	i2c_set_clientdata(client, smb);
 
 	mutex_init(&smb->lock);
 	smb->dev = &client->dev;
-	smb->pdata = pdata;
 
 	smb->regmap = devm_regmap_init_i2c(client, &smb347_regmap);
 	if (IS_ERR(smb->regmap))
@@ -1250,6 +1329,7 @@ static int smb347_probe(struct i2c_client *client,
 	mains_usb_cfg.supplied_to = battery;
 	mains_usb_cfg.num_supplicants = ARRAY_SIZE(battery);
 	mains_usb_cfg.drv_data = smb;
+	mains_usb_cfg.of_node = dev->of_node;
 	if (smb->pdata->use_mains) {
 		smb->mains = power_supply_register(dev, &smb347_mains_desc,
 						   &mains_usb_cfg);
@@ -1282,7 +1362,7 @@ static int smb347_probe(struct i2c_client *client,
 	 * Interrupt pin is optional. If it is connected, we setup the
 	 * interrupt support here.
 	 */
-	if (pdata->irq_gpio >= 0) {
+	if (smb->pdata->irq_gpio >= 0) {
 		ret = smb347_irq_init(smb, client);
 		if (ret < 0) {
 			dev_warn(dev, "failed to initialize IRQ: %d\n", ret);
@@ -1316,9 +1396,16 @@ static const struct i2c_device_id smb347_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, smb347_id);
 
+static const struct of_device_id smb3xx_of_match[] = {
+	{ .compatible = "summit,smb347" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, smb3xx_of_match);
+
 static struct i2c_driver smb347_driver = {
 	.driver = {
 		.name = "smb347",
+		.of_match_table = smb3xx_of_match,
 	},
 	.probe        = smb347_probe,
 	.remove       = smb347_remove,
-- 
2.25.0

[PATCH 6/9] power: supply: smb347-charger: Support SMB345 and SMB358

[David Heidelberg]

Tested SMB345 on Nexus 7 2013. Works.

Based on:
- https://patchwork.kernel.org/patch/4922431/
- https://patchwork.ozlabs.org/patch/666877/

Signed-off-by: David Heidelberg <david@ixit.cz>
---
 drivers/power/supply/Kconfig          |   6 +-
 drivers/power/supply/smb347-charger.c | 109 ++++++++++++++------------
 2 files changed, 62 insertions(+), 53 deletions(-)

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index f3424fdce341..2581ed8aff5a 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -598,12 +598,12 @@ config CHARGER_BQ25890
 	  Say Y to enable support for the TI BQ25890 battery charger.
 
 config CHARGER_SMB347
-	tristate "Summit Microelectronics SMB347 Battery Charger"
+	tristate "Summit Microelectronics SMB3XX Battery Charger"
 	depends on I2C
 	select REGMAP_I2C
 	help
-	  Say Y to include support for Summit Microelectronics SMB347
-	  Battery Charger.
+	  Say Y to include support for Summit Microelectronics SMB345,
+	  SMB347 or SMB358 Battery Charger.
 
 config CHARGER_TPS65090
 	tristate "TPS65090 battery charger driver"
diff --git a/drivers/power/supply/smb347-charger.c b/drivers/power/supply/smb347-charger.c
index 852d2ab566e0..0cbd0743fd91 100644
--- a/drivers/power/supply/smb347-charger.c
+++ b/drivers/power/supply/smb347-charger.c
@@ -128,6 +128,7 @@
  * @mains: power_supply instance for AC/DC power
  * @usb: power_supply instance for USB power
  * @battery: power_supply instance for battery
+ * @id: SMB charger ID
  * @mains_online: is AC/DC input connected
  * @usb_online: is USB input connected
  * @charging_enabled: is charging enabled
@@ -140,64 +141,61 @@ struct smb347_charger {
 	struct power_supply	*mains;
 	struct power_supply	*usb;
 	struct power_supply	*battery;
+	unsigned int		id;
 	bool			mains_online;
 	bool			usb_online;
 	bool			charging_enabled;
 	const struct smb347_charger_platform_data *pdata;
 };
 
-/* Fast charge current in uA */
-static const unsigned int fcc_tbl[] = {
-	700000,
-	900000,
-	1200000,
-	1500000,
-	1800000,
-	2000000,
-	2200000,
-	2500000,
+enum smb_charger_chipid {
+	SMB345,
+	SMB347,
+	SMB358,
+	NUM_CHIP_TYPES,
 };
 
+/* Fast charge current in uA */
+static const unsigned int fcc_tbl[NUM_CHIP_TYPES][8] = {
+	[SMB345] = {  200000,  450000,  600000,  900000,
+		     1300000, 1500000, 1800000, 2000000 },
+	[SMB347] = {  700000,  900000, 1200000, 1500000,
+		     1800000, 2000000, 2200000, 2500000 },
+	[SMB358] = {  200000,  450000,  600000,  900000,
+		     1300000, 1500000, 1800000, 2000000 },
+};
 /* Pre-charge current in uA */
-static const unsigned int pcc_tbl[] = {
-	100000,
-	150000,
-	200000,
-	250000,
+static const unsigned int pcc_tbl[NUM_CHIP_TYPES][4] = {
+	[SMB345] = { 150000, 250000, 350000, 450000 },
+	[SMB347] = { 100000, 150000, 200000, 250000 },
+	[SMB358] = { 150000, 250000, 350000, 450000 },
 };
 
 /* Termination current in uA */
-static const unsigned int tc_tbl[] = {
-	37500,
-	50000,
-	100000,
-	150000,
-	200000,
-	250000,
-	500000,
-	600000,
+static const unsigned int tc_tbl[NUM_CHIP_TYPES][8] = {
+	[SMB345] = {  30000,  40000,  60000,  80000,
+		     100000, 125000, 150000, 200000 },
+	[SMB347] = {  37500,  50000, 100000, 150000,
+		     200000, 250000, 500000, 600000 },
+	[SMB358] = {  30000,  40000,  60000,  80000,
+		     100000, 125000, 150000, 200000 },
 };
 
 /* Input current limit in uA */
-static const unsigned int icl_tbl[] = {
-	300000,
-	500000,
-	700000,
-	900000,
-	1200000,
-	1500000,
-	1800000,
-	2000000,
-	2200000,
-	2500000,
+static const unsigned int icl_tbl[NUM_CHIP_TYPES][10] = {
+	[SMB345] = {  300000,  500000,  700000, 1000000, 1500000,
+		     1800000, 2000000, 2000000, 2000000, 2000000 },
+	[SMB347] = {  300000,  500000,  700000,  900000, 1200000,
+		     1500000, 1800000, 2000000, 2200000, 2500000 },
+	[SMB358] = {  300000,  500000,  700000, 1000000, 1500000,
+		     1800000, 2000000, 2000000, 2000000, 2000000 },
 };
 
 /* Charge current compensation in uA */
-static const unsigned int ccc_tbl[] = {
-	250000,
-	700000,
-	900000,
-	1200000,
+static const unsigned int ccc_tbl[NUM_CHIP_TYPES][4] = {
+	[SMB345] = {  200000,  450000,  600000,  900000 },
+	[SMB347] = {  250000,  700000,  900000, 1200000 },
+	[SMB358] = {  200000,  450000,  600000,  900000 },
 };
 
 /* Convert register value to current using lookup table */
@@ -352,10 +350,11 @@ static int smb347_start_stop_charging(struct smb347_charger *smb)
 
 static int smb347_set_charge_current(struct smb347_charger *smb)
 {
+	unsigned int id = smb->id;
 	int ret;
 
 	if (smb->pdata->max_charge_current) {
-		ret = current_to_hw(fcc_tbl, ARRAY_SIZE(fcc_tbl),
+		ret = current_to_hw(fcc_tbl[id], ARRAY_SIZE(fcc_tbl[id]),
 				    smb->pdata->max_charge_current);
 		if (ret < 0)
 			return ret;
@@ -368,7 +367,7 @@ static int smb347_set_charge_current(struct smb347_charger *smb)
 	}
 
 	if (smb->pdata->pre_charge_current) {
-		ret = current_to_hw(pcc_tbl, ARRAY_SIZE(pcc_tbl),
+		ret = current_to_hw(pcc_tbl[id], ARRAY_SIZE(pcc_tbl[id]),
 				    smb->pdata->pre_charge_current);
 		if (ret < 0)
 			return ret;
@@ -381,7 +380,7 @@ static int smb347_set_charge_current(struct smb347_charger *smb)
 	}
 
 	if (smb->pdata->termination_current) {
-		ret = current_to_hw(tc_tbl, ARRAY_SIZE(tc_tbl),
+		ret = current_to_hw(tc_tbl[id], ARRAY_SIZE(tc_tbl[id]),
 				    smb->pdata->termination_current);
 		if (ret < 0)
 			return ret;
@@ -397,10 +396,11 @@ static int smb347_set_charge_current(struct smb347_charger *smb)
 
 static int smb347_set_current_limits(struct smb347_charger *smb)
 {
+	unsigned int id = smb->id;
 	int ret;
 
 	if (smb->pdata->mains_current_limit) {
-		ret = current_to_hw(icl_tbl, ARRAY_SIZE(icl_tbl),
+		ret = current_to_hw(icl_tbl[id], ARRAY_SIZE(icl_tbl[id]),
 				    smb->pdata->mains_current_limit);
 		if (ret < 0)
 			return ret;
@@ -413,7 +413,7 @@ static int smb347_set_current_limits(struct smb347_charger *smb)
 	}
 
 	if (smb->pdata->usb_hc_current_limit) {
-		ret = current_to_hw(icl_tbl, ARRAY_SIZE(icl_tbl),
+		ret = current_to_hw(icl_tbl[id], ARRAY_SIZE(icl_tbl[id]),
 				    smb->pdata->usb_hc_current_limit);
 		if (ret < 0)
 			return ret;
@@ -463,6 +463,7 @@ static int smb347_set_voltage_limits(struct smb347_charger *smb)
 
 static int smb347_set_temp_limits(struct smb347_charger *smb)
 {
+	unsigned int id = smb->id;
 	bool enable_therm_monitor = false;
 	int ret = 0;
 	int val;
@@ -587,7 +588,7 @@ static int smb347_set_temp_limits(struct smb347_charger *smb)
 	}
 
 	if (smb->pdata->charge_current_compensation) {
-		val = current_to_hw(ccc_tbl, ARRAY_SIZE(ccc_tbl),
+		val = current_to_hw(ccc_tbl[id], ARRAY_SIZE(ccc_tbl[id]),
 				    smb->pdata->charge_current_compensation);
 		if (val < 0)
 			return val;
@@ -883,6 +884,7 @@ static int smb347_irq_init(struct smb347_charger *smb,
  */
 static int get_const_charge_current(struct smb347_charger *smb)
 {
+	unsigned int id = smb->id;
 	int ret, intval;
 	unsigned int v;
 
@@ -898,10 +900,12 @@ static int get_const_charge_current(struct smb347_charger *smb)
 	 * and we can detect which table to use from bit 5.
 	 */
 	if (v & 0x20) {
-		intval = hw_to_current(fcc_tbl, ARRAY_SIZE(fcc_tbl), v & 7);
+		intval = hw_to_current(fcc_tbl[id],
+				       ARRAY_SIZE(fcc_tbl[id]), v & 7);
 	} else {
 		v >>= 3;
-		intval = hw_to_current(pcc_tbl, ARRAY_SIZE(pcc_tbl), v & 7);
+		intval = hw_to_current(pcc_tbl[id],
+				       ARRAY_SIZE(pcc_tbl[id]), v & 7);
 	}
 
 	return intval;
@@ -1317,6 +1321,7 @@ static int smb347_probe(struct i2c_client *client,
 
 	mutex_init(&smb->lock);
 	smb->dev = &client->dev;
+	smb->id = id->driver_data;
 
 	smb->regmap = devm_regmap_init_i2c(client, &smb347_regmap);
 	if (IS_ERR(smb->regmap))
@@ -1391,13 +1396,17 @@ static int smb347_remove(struct i2c_client *client)
 }
 
 static const struct i2c_device_id smb347_id[] = {
-	{ "smb347", 0 },
-	{ }
+	{ "smb345", SMB345 },
+	{ "smb347", SMB347 },
+	{ "smb358", SMB358 },
+	{}
 };
 MODULE_DEVICE_TABLE(i2c, smb347_id);
 
 static const struct of_device_id smb3xx_of_match[] = {
+	{ .compatible = "summit,smb345" },
 	{ .compatible = "summit,smb347" },
+	{ .compatible = "summit,smb358" },
 	{},
 };
 MODULE_DEVICE_TABLE(of, smb3xx_of_match);
-- 
2.25.0

[PATCH 7/9] power: supply: smb347-charger: Remove virtual smb347-battery

[David Heidelberg]

smb347 is a charger and not a battery driver, and that power-supply core
now supports monitored-battery. So the 'fake' battery doesn't do anything
useful for us, and thus, it should be removed.

Transfer this functionality into smb347-mains and smb347-usb.

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: David Heidelberg <david@ixit.cz>
---
 drivers/power/supply/smb347-charger.c | 212 ++++++++------------------
 1 file changed, 60 insertions(+), 152 deletions(-)

diff --git a/drivers/power/supply/smb347-charger.c b/drivers/power/supply/smb347-charger.c
index 0cbd0743fd91..ce2ebfe601d6 100644
--- a/drivers/power/supply/smb347-charger.c
+++ b/drivers/power/supply/smb347-charger.c
@@ -127,7 +127,6 @@
  * @regmap: pointer to driver regmap
  * @mains: power_supply instance for AC/DC power
  * @usb: power_supply instance for USB power
- * @battery: power_supply instance for battery
  * @id: SMB charger ID
  * @mains_online: is AC/DC input connected
  * @usb_online: is USB input connected
@@ -140,7 +139,6 @@ struct smb347_charger {
 	struct regmap		*regmap;
 	struct power_supply	*mains;
 	struct power_supply	*usb;
-	struct power_supply	*battery;
 	unsigned int		id;
 	bool			mains_online;
 	bool			usb_online;
@@ -743,7 +741,10 @@ static irqreturn_t smb347_interrupt(int irq, void *data)
 	 */
 	if (stat_c & STAT_C_CHARGER_ERROR) {
 		dev_err(smb->dev, "charging stopped due to charger error\n");
-		power_supply_changed(smb->battery);
+		if (smb->pdata->use_mains)
+			power_supply_changed(smb->mains);
+		if (smb->pdata->use_usb)
+			power_supply_changed(smb->usb);
 		handled = true;
 	}
 
@@ -753,8 +754,12 @@ static irqreturn_t smb347_interrupt(int irq, void *data)
 	 * disabled by the hardware.
 	 */
 	if (irqstat_c & (IRQSTAT_C_TERMINATION_IRQ | IRQSTAT_C_TAPER_IRQ)) {
-		if (irqstat_c & IRQSTAT_C_TERMINATION_STAT)
-			power_supply_changed(smb->battery);
+		if (irqstat_c & IRQSTAT_C_TERMINATION_STAT) {
+			if (smb->pdata->use_mains)
+				power_supply_changed(smb->mains);
+			if (smb->pdata->use_usb)
+				power_supply_changed(smb->usb);
+		}
 		dev_dbg(smb->dev, "going to HW maintenance mode\n");
 		handled = true;
 	}
@@ -768,7 +773,10 @@ static irqreturn_t smb347_interrupt(int irq, void *data)
 
 		if (irqstat_d & IRQSTAT_D_CHARGE_TIMEOUT_STAT)
 			dev_warn(smb->dev, "charging stopped due to timeout\n");
-		power_supply_changed(smb->battery);
+		if (smb->pdata->use_mains)
+			power_supply_changed(smb->mains);
+		if (smb->pdata->use_usb)
+			power_supply_changed(smb->usb);
 		handled = true;
 	}
 
@@ -936,95 +944,19 @@ static int get_const_charge_voltage(struct smb347_charger *smb)
 	return intval;
 }
 
-static int smb347_mains_get_property(struct power_supply *psy,
-				     enum power_supply_property prop,
-				     union power_supply_propval *val)
-{
-	struct smb347_charger *smb = power_supply_get_drvdata(psy);
-	int ret;
-
-	switch (prop) {
-	case POWER_SUPPLY_PROP_ONLINE:
-		val->intval = smb->mains_online;
-		break;
-
-	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
-		ret = get_const_charge_voltage(smb);
-		if (ret < 0)
-			return ret;
-		else
-			val->intval = ret;
-		break;
-
-	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
-		ret = get_const_charge_current(smb);
-		if (ret < 0)
-			return ret;
-		else
-			val->intval = ret;
-		break;
-
-	default:
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
-static enum power_supply_property smb347_mains_properties[] = {
-	POWER_SUPPLY_PROP_ONLINE,
-	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
-	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
-};
-
-static int smb347_usb_get_property(struct power_supply *psy,
-				   enum power_supply_property prop,
-				   union power_supply_propval *val)
-{
-	struct smb347_charger *smb = power_supply_get_drvdata(psy);
-	int ret;
-
-	switch (prop) {
-	case POWER_SUPPLY_PROP_ONLINE:
-		val->intval = smb->usb_online;
-		break;
-
-	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
-		ret = get_const_charge_voltage(smb);
-		if (ret < 0)
-			return ret;
-		else
-			val->intval = ret;
-		break;
-
-	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
-		ret = get_const_charge_current(smb);
-		if (ret < 0)
-			return ret;
-		else
-			val->intval = ret;
-		break;
-
-	default:
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
-static enum power_supply_property smb347_usb_properties[] = {
-	POWER_SUPPLY_PROP_ONLINE,
-	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
-	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
-};
-
-static int smb347_get_charging_status(struct smb347_charger *smb)
+static int smb347_get_charging_status(struct smb347_charger *smb,
+				      struct power_supply *psy)
 {
 	int ret, status;
 	unsigned int val;
 
-	if (!smb347_is_ps_online(smb))
-		return POWER_SUPPLY_STATUS_DISCHARGING;
+	if (psy->desc->type == POWER_SUPPLY_TYPE_USB) {
+		if (!smb->usb_online)
+			return POWER_SUPPLY_STATUS_DISCHARGING;
+	} else {
+		if (!smb->mains_online)
+			return POWER_SUPPLY_STATUS_DISCHARGING;
+	}
 
 	ret = regmap_read(smb->regmap, STAT_C, &val);
 	if (ret < 0)
@@ -1063,29 +995,29 @@ static int smb347_get_charging_status(struct smb347_charger *smb)
 	return status;
 }
 
-static int smb347_battery_get_property(struct power_supply *psy,
-				       enum power_supply_property prop,
-				       union power_supply_propval *val)
+static int smb347_get_property(struct power_supply *psy,
+			       enum power_supply_property prop,
+			       union power_supply_propval *val)
 {
 	struct smb347_charger *smb = power_supply_get_drvdata(psy);
-	const struct smb347_charger_platform_data *pdata = smb->pdata;
 	int ret;
 
-	ret = smb347_update_ps_status(smb);
-	if (ret < 0)
-		return ret;
-
 	switch (prop) {
 	case POWER_SUPPLY_PROP_STATUS:
-		ret = smb347_get_charging_status(smb);
+		ret = smb347_get_charging_status(smb, psy);
 		if (ret < 0)
 			return ret;
 		val->intval = ret;
 		break;
 
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
-		if (!smb347_is_ps_online(smb))
-			return -ENODATA;
+		if (psy->desc->type == POWER_SUPPLY_TYPE_USB) {
+			if (!smb->usb_online)
+				return -ENODATA;
+		} else {
+			if (!smb->mains_online)
+				return -ENODATA;
+		}
 
 		/*
 		 * We handle trickle and pre-charging the same, and taper
@@ -1104,24 +1036,25 @@ static int smb347_battery_get_property(struct power_supply *psy,
 		}
 		break;
 
-	case POWER_SUPPLY_PROP_TECHNOLOGY:
-		val->intval = pdata->battery_info.technology;
-		break;
-
-	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
-		val->intval = pdata->battery_info.voltage_min_design;
-		break;
-
-	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
-		val->intval = pdata->battery_info.voltage_max_design;
+	case POWER_SUPPLY_PROP_ONLINE:
+		if (psy->desc->type == POWER_SUPPLY_TYPE_USB)
+			val->intval = smb->usb_online;
+		else
+			val->intval = smb->mains_online;
 		break;
 
-	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
-		val->intval = pdata->battery_info.charge_full_design;
+	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
+		ret = get_const_charge_voltage(smb);
+		if (ret < 0)
+			return ret;
+		val->intval = ret;
 		break;
 
-	case POWER_SUPPLY_PROP_MODEL_NAME:
-		val->strval = pdata->battery_info.name;
+	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
+		ret = get_const_charge_current(smb);
+		if (ret < 0)
+			return ret;
+		val->intval = ret;
 		break;
 
 	default:
@@ -1131,14 +1064,12 @@ static int smb347_battery_get_property(struct power_supply *psy,
 	return 0;
 }
 
-static enum power_supply_property smb347_battery_properties[] = {
+static enum power_supply_property smb347_properties[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_CHARGE_TYPE,
-	POWER_SUPPLY_PROP_TECHNOLOGY,
-	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
-	POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
-	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
-	POWER_SUPPLY_PROP_MODEL_NAME,
+	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
+	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
 };
 
 static bool smb347_volatile_reg(struct device *dev, unsigned int reg)
@@ -1276,32 +1207,23 @@ static const struct regmap_config smb347_regmap = {
 static const struct power_supply_desc smb347_mains_desc = {
 	.name		= "smb347-mains",
 	.type		= POWER_SUPPLY_TYPE_MAINS,
-	.get_property	= smb347_mains_get_property,
-	.properties	= smb347_mains_properties,
-	.num_properties	= ARRAY_SIZE(smb347_mains_properties),
+	.get_property	= smb347_get_property,
+	.properties	= smb347_properties,
+	.num_properties	= ARRAY_SIZE(smb347_properties),
 };
 
 static const struct power_supply_desc smb347_usb_desc = {
 	.name		= "smb347-usb",
 	.type		= POWER_SUPPLY_TYPE_USB,
-	.get_property	= smb347_usb_get_property,
-	.properties	= smb347_usb_properties,
-	.num_properties	= ARRAY_SIZE(smb347_usb_properties),
-};
-
-static const struct power_supply_desc smb347_battery_desc = {
-	.name		= "smb347-battery",
-	.type		= POWER_SUPPLY_TYPE_BATTERY,
-	.get_property	= smb347_battery_get_property,
-	.properties	= smb347_battery_properties,
-	.num_properties	= ARRAY_SIZE(smb347_battery_properties),
+	.get_property	= smb347_get_property,
+	.properties	= smb347_properties,
+	.num_properties	= ARRAY_SIZE(smb347_properties),
 };
 
 static int smb347_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
-	static char *battery[] = { "smb347-battery" };
-	struct power_supply_config mains_usb_cfg = {}, battery_cfg = {};
+	struct power_supply_config mains_usb_cfg = {};
 	struct device *dev = &client->dev;
 	struct smb347_charger *smb;
 	int ret;
@@ -1331,8 +1253,6 @@ static int smb347_probe(struct i2c_client *client,
 	if (ret < 0)
 		return ret;
 
-	mains_usb_cfg.supplied_to = battery;
-	mains_usb_cfg.num_supplicants = ARRAY_SIZE(battery);
 	mains_usb_cfg.drv_data = smb;
 	mains_usb_cfg.of_node = dev->of_node;
 	if (smb->pdata->use_mains) {
@@ -1352,17 +1272,6 @@ static int smb347_probe(struct i2c_client *client,
 		}
 	}
 
-	battery_cfg.drv_data = smb;
-	smb->battery = power_supply_register(dev, &smb347_battery_desc,
-					     &battery_cfg);
-	if (IS_ERR(smb->battery)) {
-		if (smb->pdata->use_usb)
-			power_supply_unregister(smb->usb);
-		if (smb->pdata->use_mains)
-			power_supply_unregister(smb->mains);
-		return PTR_ERR(smb->battery);
-	}
-
 	/*
 	 * Interrupt pin is optional. If it is connected, we setup the
 	 * interrupt support here.
@@ -1387,7 +1296,6 @@ static int smb347_remove(struct i2c_client *client)
 	if (client->irq)
 		smb347_irq_disable(smb);
 
-	power_supply_unregister(smb->battery);
 	if (smb->pdata->use_usb)
 		power_supply_unregister(smb->usb);
 	if (smb->pdata->use_mains)
-- 
2.25.0

[PATCH 8/9] power: supply: smb347-charger: Replace mutex with IRQ disable/enable

[David Heidelberg]

From: Dmitry Osipenko <digetx@gmail.com>

Rather properly disable/enable IRQ than use mutex.
This patch makes code easier to follow.

Tested-by: David Heidelberg <david@ixit.cz>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: David Heidelberg <david@ixit.cz>
---
 drivers/power/supply/smb347-charger.c | 38 ++++++++++++++-------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/power/supply/smb347-charger.c b/drivers/power/supply/smb347-charger.c
index ce2ebfe601d6..60a0ca2d6d74 100644
--- a/drivers/power/supply/smb347-charger.c
+++ b/drivers/power/supply/smb347-charger.c
@@ -16,7 +16,6 @@
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/i2c.h>
-#include <linux/mutex.h>
 #include <linux/power_supply.h>
 #include <linux/power/smb347-charger.h>
 #include <linux/regmap.h>
@@ -122,7 +121,6 @@
 
 /**
  * struct smb347_charger - smb347 charger instance
- * @lock: protects concurrent access to online variables
  * @dev: pointer to device
  * @regmap: pointer to driver regmap
  * @mains: power_supply instance for AC/DC power
@@ -134,7 +132,6 @@
  * @pdata: pointer to platform data
  */
 struct smb347_charger {
-	struct mutex		lock;
 	struct device		*dev;
 	struct regmap		*regmap;
 	struct power_supply	*mains;
@@ -243,11 +240,9 @@ static int smb347_update_ps_status(struct smb347_charger *smb)
 	if (smb->pdata->use_usb)
 		usb = !(val & IRQSTAT_E_USBIN_UV_STAT);
 
-	mutex_lock(&smb->lock);
 	ret = smb->mains_online != dc || smb->usb_online != usb;
 	smb->mains_online = dc;
 	smb->usb_online = usb;
-	mutex_unlock(&smb->lock);
 
 	return ret;
 }
@@ -263,13 +258,7 @@ static int smb347_update_ps_status(struct smb347_charger *smb)
  */
 static bool smb347_is_ps_online(struct smb347_charger *smb)
 {
-	bool ret;
-
-	mutex_lock(&smb->lock);
-	ret = smb->usb_online || smb->mains_online;
-	mutex_unlock(&smb->lock);
-
-	return ret;
+	return smb->usb_online || smb->mains_online;
 }
 
 /**
@@ -303,14 +292,13 @@ static int smb347_charging_set(struct smb347_charger *smb, bool enable)
 		return 0;
 	}
 
-	mutex_lock(&smb->lock);
 	if (smb->charging_enabled != enable) {
 		ret = regmap_update_bits(smb->regmap, CMD_A, CMD_A_CHG_ENABLED,
 					 enable ? CMD_A_CHG_ENABLED : 0);
 		if (!ret)
 			smb->charging_enabled = enable;
 	}
-	mutex_unlock(&smb->lock);
+
 	return ret;
 }
 
@@ -995,9 +983,9 @@ static int smb347_get_charging_status(struct smb347_charger *smb,
 	return status;
 }
 
-static int smb347_get_property(struct power_supply *psy,
-			       enum power_supply_property prop,
-			       union power_supply_propval *val)
+static int smb347_get_property_locked(struct power_supply *psy,
+				      enum power_supply_property prop,
+				      union power_supply_propval *val)
 {
 	struct smb347_charger *smb = power_supply_get_drvdata(psy);
 	int ret;
@@ -1064,6 +1052,21 @@ static int smb347_get_property(struct power_supply *psy,
 	return 0;
 }
 
+static int smb347_get_property(struct power_supply *psy,
+			       enum power_supply_property prop,
+			       union power_supply_propval *val)
+{
+	struct smb347_charger *smb = power_supply_get_drvdata(psy);
+	struct i2c_client *client = to_i2c_client(smb->dev);
+	int ret;
+
+	disable_irq(client->irq);
+	ret = smb347_get_property_locked(psy, prop, val);
+	enable_irq(client->irq);
+
+	return ret;
+}
+
 static enum power_supply_property smb347_properties[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_CHARGE_TYPE,
@@ -1241,7 +1244,6 @@ static int smb347_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, smb);
 
-	mutex_init(&smb->lock);
 	smb->dev = &client->dev;
 	smb->id = id->driver_data;
 
-- 
2.25.0

[PATCH 9/9] arm: dts: qcom: apq8064-nexus7: Add smb345 charger node

[David Heidelberg]

Add smb345 charger node to Nexus 7 2013 DTS.
Proper charger initialization also prevents battery from overcharging.

Original author: Vinay Simha BN <simhavcs@gmail.com>

Signed-off-by: David Heidelberg <david@ixit.cz>
---
 .../boot/dts/qcom-apq8064-asus-nexus7-flo.dts | 22 ++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/qcom-apq8064-asus-nexus7-flo.dts b/arch/arm/boot/dts/qcom-apq8064-asus-nexus7-flo.dts
index a701d4bac320..9f14216a22f1 100644
--- a/arch/arm/boot/dts/qcom-apq8064-asus-nexus7-flo.dts
+++ b/arch/arm/boot/dts/qcom-apq8064-asus-nexus7-flo.dts
@@ -3,6 +3,7 @@
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/input/input.h>
 #include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
+#include <dt-bindings/power/summit,smb347-charger.h>
 / {
 	model = "Asus Nexus7(flo)";
 	compatible = "asus,nexus7-flo", "qcom,apq8064";
@@ -293,11 +294,30 @@ eeprom@52 {
 					pagesize = <32>;
 				};
 
-				bq27541@55 {
+				bat: battery@55 {
 					compatible = "ti,bq27541";
 					reg = <0x55>;
+					power-supplies = <&power_supply>;
 				};
 
+				power_supply: charger@6a {
+					compatible = "summit,smb345";
+					reg = <0x6a>;
+
+					interrupt-parent = <&tlmm_pinmux>;
+					interrupts = <23 IRQ_TYPE_EDGE_BOTH>;
+
+					summit,max-chg-curr = <1800000>;
+					summit,usb-curr-limit = <500000>;
+
+					summit,chip-temp-threshold = <110>;
+
+					summit,enable-usb-charging;
+					summit,enable-otg-charging;
+
+					summit,enable-chg-ctrl =
+						<SMB3XX_CHG_ENABLE_SW>;
+				};
 			};
 		};
 
-- 
2.25.0

Re: [PATCH 5/9] power: supply: smb347-charger: Implement device-tree support

[Jonghwa Lee]

On 20. 3. 30. 오전 1:21, David Heidelberg wrote:
> This patch makes smb347 charger driver to support dt binding. All legacy
> platform data now can be parsed from dt.
> Because of that smb347 is i2c client driver, IRQ number can be passed
> automatically through client's irq variable if it is defined in dt.
> No more to use requesting gpio to irq manually in dt-way.
Thanks for keeping original commit description and revealing where it 
comes from.

Anyway, in the above description, the last three lines doesn't match to 
the commit since

the original patch is now split. Please remove them or you can rewrite 
description as your own.

>
> Based on: https://patchwork.kernel.org/patch/4284731/
> Original author: Jonghwa Lee <jonghwa3.lee@samsung.com>
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> Signed-off-by: David Heidelberg <david@ixit.cz>
> ---
>   drivers/power/supply/smb347-charger.c | 107 +++++++++++++++++++++++---
>   1 file changed, 97 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/power/supply/smb347-charger.c b/drivers/power/supply/smb347-charger.c
> index 4add9f64ba90..852d2ab566e0 100644
> --- a/drivers/power/supply/smb347-charger.c
> +++ b/drivers/power/supply/smb347-charger.c
> @@ -1180,6 +1180,87 @@ static bool smb347_readable_reg(struct device *dev, unsigned int reg)
>   	return smb347_volatile_reg(dev, reg);
>   }
>   
> +static void smb347_dt_parse_pdata(struct device_node *np,
> +				  struct smb347_charger_platform_data *pdata)
> +{
> +	/* Charging constraints */
> +	of_property_read_u32(np, "summit,max-chg-curr",
> +			     &pdata->max_charge_current);
> +	of_property_read_u32(np, "summit,max-chg-volt",
> +			     &pdata->max_charge_voltage);
> +	of_property_read_u32(np, "summit,pre-chg-curr",
> +			     &pdata->pre_charge_current);
> +	of_property_read_u32(np, "summit,term-curr",
> +			     &pdata->termination_current);
> +	of_property_read_u32(np, "summit,fast-volt-threshold",
> +			     &pdata->pre_to_fast_voltage);
> +	of_property_read_u32(np, "summit,mains-curr-limit",
> +			     &pdata->mains_current_limit);
> +	of_property_read_u32(np, "summit,usb-curr-limit",
> +			     &pdata->usb_hc_current_limit);
> +
> +	/* For thermometer monitoring */
> +	of_property_read_u32(np, "summit,chip-temp-threshold",
> +			     &pdata->chip_temp_threshold);
> +	if (of_property_read_u32(np, "summit,soft-cold-temp-limit",
> +				 &pdata->soft_cold_temp_limit))
> +		pdata->soft_cold_temp_limit = SMB347_TEMP_USE_DEFAULT;
> +	if (of_property_read_u32(np, "summit,soft-hot-temp-limit",
> +				 &pdata->soft_hot_temp_limit))
> +		pdata->soft_hot_temp_limit = SMB347_TEMP_USE_DEFAULT;
> +	if (of_property_read_u32(np, "summit,hard-cold-temp-limit",
> +				 &pdata->hard_cold_temp_limit))
> +		pdata->hard_cold_temp_limit = SMB347_TEMP_USE_DEFAULT;
> +	if (of_property_read_u32(np, "summit,hard-hot-temp-limit",
> +				 &pdata->hard_hot_temp_limit))
> +		pdata->hard_hot_temp_limit = SMB347_TEMP_USE_DEFAULT;
> +
> +	/* Suspend when battery temperature is outside hard limits */
> +	if (pdata->hard_cold_temp_limit != SMB347_TEMP_USE_DEFAULT ||
> +	    pdata->hard_hot_temp_limit != SMB347_TEMP_USE_DEFAULT)
> +		pdata->suspend_on_hard_temp_limit = true;
> +
> +	if (of_property_read_u32(np, "summit,soft-comp-method",
> +				 &pdata->soft_temp_limit_compensation))
> +		pdata->soft_temp_limit_compensation =
> +				SMB347_SOFT_TEMP_COMPENSATE_DEFAULT;
> +
> +	of_property_read_u32(np, "summit,chg-curr-comp",
> +			     &pdata->charge_current_compensation);
> +
> +	/* Supported charging mode */
> +	pdata->use_mains =
> +		of_property_read_bool(np, "summit,enable-mains-charging");
> +	pdata->use_usb =
> +		of_property_read_bool(np, "summit,enable-usb-charging");
> +	pdata->use_usb_otg =
> +		of_property_read_bool(np, "summit,enable-otg-charging");
> +
> +	/* Enable charging method */
> +	of_property_read_u32(np, "summit,enable-chg-ctrl",
> +			     &pdata->enable_control);
> +
> +	/* If IRQ is enabled or not */
> +	if (!of_find_property(np, "interrupts", NULL))
> +		pdata->irq_gpio = -1;
> +}
> +
> +static struct smb347_charger_platform_data
> +			*smb347_get_platdata(struct device *dev)
> +{
> +	struct smb347_charger_platform_data *pdata;
> +
> +	if (dev->of_node) {
> +		pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +		if (pdata)
> +			smb347_dt_parse_pdata(dev->of_node, pdata);
> +	} else {
> +		pdata = dev_get_platdata(dev);
> +	}
> +
> +	return pdata;
> +}
> +
>   static const struct regmap_config smb347_regmap = {
>   	.reg_bits	= 8,
>   	.val_bits	= 8,
> @@ -1216,28 +1297,26 @@ static int smb347_probe(struct i2c_client *client,
>   			const struct i2c_device_id *id)
>   {
>   	static char *battery[] = { "smb347-battery" };
> -	const struct smb347_charger_platform_data *pdata;
>   	struct power_supply_config mains_usb_cfg = {}, battery_cfg = {};
>   	struct device *dev = &client->dev;
>   	struct smb347_charger *smb;
>   	int ret;
>   
> -	pdata = dev->platform_data;
> -	if (!pdata)
> -		return -EINVAL;
> -
> -	if (!pdata->use_mains && !pdata->use_usb)
> -		return -EINVAL;
> -
>   	smb = devm_kzalloc(dev, sizeof(*smb), GFP_KERNEL);
>   	if (!smb)
>   		return -ENOMEM;
>   
> +	smb->pdata = smb347_get_platdata(dev);
> +	if (!smb->pdata)
> +		return -ENODEV;
> +
> +	if (!smb->pdata->use_mains && !smb->pdata->use_usb)
> +		return -EINVAL;
> +
>   	i2c_set_clientdata(client, smb);
>   
>   	mutex_init(&smb->lock);
>   	smb->dev = &client->dev;
> -	smb->pdata = pdata;
>   
>   	smb->regmap = devm_regmap_init_i2c(client, &smb347_regmap);
>   	if (IS_ERR(smb->regmap))
> @@ -1250,6 +1329,7 @@ static int smb347_probe(struct i2c_client *client,
>   	mains_usb_cfg.supplied_to = battery;
>   	mains_usb_cfg.num_supplicants = ARRAY_SIZE(battery);
>   	mains_usb_cfg.drv_data = smb;
> +	mains_usb_cfg.of_node = dev->of_node;
>   	if (smb->pdata->use_mains) {
>   		smb->mains = power_supply_register(dev, &smb347_mains_desc,
>   						   &mains_usb_cfg);
> @@ -1282,7 +1362,7 @@ static int smb347_probe(struct i2c_client *client,
>   	 * Interrupt pin is optional. If it is connected, we setup the
>   	 * interrupt support here.
>   	 */
> -	if (pdata->irq_gpio >= 0) {
> +	if (smb->pdata->irq_gpio >= 0) {
>   		ret = smb347_irq_init(smb, client);
>   		if (ret < 0) {
>   			dev_warn(dev, "failed to initialize IRQ: %d\n", ret);
> @@ -1316,9 +1396,16 @@ static const struct i2c_device_id smb347_id[] = {
>   };
>   MODULE_DEVICE_TABLE(i2c, smb347_id);
>   
> +static const struct of_device_id smb3xx_of_match[] = {
> +	{ .compatible = "summit,smb347" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, smb3xx_of_match);
> +
>   static struct i2c_driver smb347_driver = {
>   	.driver = {
>   		.name = "smb347",
> +		.of_match_table = smb3xx_of_match,
>   	},
>   	.probe        = smb347_probe,
>   	.remove       = smb347_remove,

Re: [PATCH 5/9] power: supply: smb347-charger: Implement device-tree support

[Dmitry Osipenko]

Hello Jonghwa,

31.03.2020 03:25, Jonghwa Lee пишет:
> On 20. 3. 30. 오전 1:21, David Heidelberg wrote:
>> This patch makes smb347 charger driver to support dt binding. All legacy
>> platform data now can be parsed from dt.
>> Because of that smb347 is i2c client driver, IRQ number can be passed
>> automatically through client's irq variable if it is defined in dt.
>> No more to use requesting gpio to irq manually in dt-way.
> Thanks for keeping original commit description and revealing where it 
> comes from.
> 
> Anyway, in the above description, the last three lines doesn't match to 
> the commit since
> 
> the original patch is now split. Please remove them or you can rewrite 
> description as your own.

That's a good suggestion, thank you very much.

Re: [PATCH 4/9] dt-bindings: power: supply: Add device-tree binding for Summit SMB3xx

[Rob Herring]

On Sun, Mar 29, 2020 at 06:21:23PM +0200, David Heidelberg wrote:
> Summit SMB3xx series is a Programmable Switching Li+ Battery Charger.
> This device-tree binding documents Summit SMB345, SMB347 and SMB358 chargers.
> 
> Signed-off-by: David Heidelberg <david@ixit.cz>
> ---
>  .../power/supply/summit,smb347-charger.yaml   | 224 ++++++++++++++++++
>  .../dt-bindings/power/summit,smb347-charger.h |  19 ++
>  2 files changed, 243 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/supply/summit,smb347-charger.yaml
>  create mode 100644 include/dt-bindings/power/summit,smb347-charger.h
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/summit,smb347-charger.yaml b/Documentation/devicetree/bindings/power/supply/summit,smb347-charger.yaml
> new file mode 100644
> index 000000000000..1d6bccdcd233
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/summit,smb347-charger.yaml
> @@ -0,0 +1,224 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/power/supply/summit,smb347-charger.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Battery charger driver for SMB345, SMB347 and SMB358
> +
> +maintainers:
> +  - David Heidelberg <david@ixit.cz>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - summit,smb345
> +      - summit,smb347
> +      - summit,smb358
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  summit,enable-usb-charging:
> +    type: boolean
> +    description: Enable charging trough USB.
> +
> +  summit,enable-otg-charging:
> +    type: boolean
> +    description: Provide power for USB OTG
> +
> +  summit,enable-mains-charging:
> +    type: boolean
> +    description: Enable charging trough mains
> +
> +  summit,enable-chg-ctrl:
> +    description: Enable charging control
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +      - enum:
> +          - 0 # SMB3XX_CHG_ENABLE_SW SW (I2C interface)
> +          - 1 # SMB3XX_CHG_ENABLE_PIN_ACTIVE_LOW Pin control (Active Low)
> +          - 2 # SMB3XX_CHG_ENABLE_PIN_ACTIVE_HIGH Pin control (Active High)
> +
> +  summit,max-chg-curr:
> +    description: Maximum current for charging (in uA)
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  summit,max-chg-volt:
> +    description: Maximum voltage for charging (in uV)
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 3500000
> +    maximum: 4500000
> +
> +  summit,pre-chg-curr:
> +    description: Pre-charging current for charging (in uA)
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  summit,term-curr:
> +    description: Charging cycle termination current (in uA)
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  summit,fast-volt-threshold:
> +    description: Voltage threshold to transit to fast charge mode (in uV)
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 2400000
> +    maximum: 3000000
> +
> +  summit,mains-curr-limit:
> +    description: Maximum input current from AC/DC input (in uA)
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  summit,usb-curr-limit:
> +    description: Maximum input current from USB input (in uA)
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32

These are all properties of the battery attached and we have standard 
properties for some/all of these.

Also, any property with units should have a unit suffix as defined in 
property-units.txt. And with that, you don't need to define the type.

> +
> +  summit,chg-curr-comp:
> +    description: Charge current compensation (in uA)
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  summit,chip-temp-threshold:
> +    description: Chip temperature for thermal regulation in °C.
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [100, 110, 120, 130]
> +
> +  summit,soft-cold-temp-limit:
> +    description: Cold battery temperature in °C for soft alarm.
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 5, 10, 15]
> +
> +  summit,soft-hot-temp-limit:
> +    description: Hot battery temperature in °C for soft alarm.
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [40, 45, 50, 55]
> +
> +  summit,hard-cold-temp-limit:
> +    description: Cold battery temperature in °C for hard alarm.
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/int32
> +    enum: [-5, 0, 5, 10]
> +
> +  summit,hard-hot-temp-limit:
> +    description: Hot battery temperature in °C for hard alarm.
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [50, 55, 60, 65]
> +
> +  summit,soft-comp-method:
> +    description: Soft temperature limit compensation method
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +      - enum:
> +          - 0 # SMB3XX_SOFT_TEMP_COMPENSATE_NONE Compensation none
> +          - 1 # SMB3XX_SOFT_TEMP_COMPENSATE_CURRENT Current compensation
> +          - 2 # SMB3XX_SOFT_TEMP_COMPENSATE_VOLTAGE Voltage compensation
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          enum:
> +            - summit,smb345
> +            - summit,smb358
> +
> +    then:
> +      properties:
> +        summit,max-chg-curr:
> +          enum: [ 200000,  450000,  600000,  900000,
> +                 1300000, 1500000, 1800000, 2000000]
> +
> +        summit,pre-chg-curr:
> +          enum: [150000, 250000, 350000, 450000]
> +
> +        summit,term-curr:
> +          enum: [ 30000,  40000,  60000,  80000,
> +                 100000, 125000, 150000, 200000]
> +
> +        summit,mains-curr-limit:
> +          enum: [ 300000,  500000,  700000, 1000000,
> +                 1500000, 1800000, 2000000]
> +
> +        summit,usb-curr-limit:
> +          enum: [ 300000,  500000,  700000, 1000000,
> +                 1500000, 1800000, 2000000]
> +
> +        summit,chg-curr-comp:
> +          enum: [200000, 450000, 600000, 900000]
> +
> +    else:
> +      properties:
> +        summit,max-chg-curr:
> +          enum: [ 700000,  900000, 1200000, 1500000,
> +                 1800000, 2000000, 2200000, 2500000]
> +
> +        summit,pre-chg-curr:
> +          enum: [100000, 150000, 200000, 250000]
> +
> +        summit,term-curr:
> +          enum: [ 37500,  50000, 100000, 150000,
> +                 200000, 250000, 500000, 600000]
> +
> +        summit,mains-curr-limit:
> +          enum: [ 300000,  500000,  700000,  900000, 1200000,
> +                 1500000, 1800000, 2000000, 2200000, 2500000]
> +
> +        summit,usb-curr-limit:
> +          enum: [ 300000,  500000,  700000,  900000, 1200000,
> +                 1500000, 1800000, 2000000, 2200000, 2500000]
> +
> +        summit,chg-curr-comp:
> +          enum: [250000, 700000, 900000, 1200000]
> +
> +required:
> +  - compatible
> +  - reg
> +
> +anyOf:
> +  - required:
> +      - summit,enable-usb-charging
> +  - required:
> +      - summit,enable-otg-charging
> +  - required:
> +      - summit,enable-mains-charging
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/power/summit,smb347-charger.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        charger@7f {
> +            compatible = "summit,smb347";
> +            reg = <0x7f>;
> +            status = "okay";
> +
> +            summit,max-chg-curr = <1800000>;
> +            summit,mains-curr-limit = <2000000>;
> +            summit,usb-curr-limit = <500000>;
> +
> +            summit,chip-temp-threshold = <110>;
> +            summit,soft-cold-temp-limit = <5>;
> +
> +            summit,enable-usb-charging;
> +            summit,enable-mains-charging;
> +
> +            summit,enable-chg-ctrl = <SMB3XX_CHG_ENABLE_PIN_ACTIVE_HIGH>;
> +        };
> +    };
> diff --git a/include/dt-bindings/power/summit,smb347-charger.h b/include/dt-bindings/power/summit,smb347-charger.h
> new file mode 100644
> index 000000000000..d918bf321a71
> --- /dev/null
> +++ b/include/dt-bindings/power/summit,smb347-charger.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: (GPL-2.0-or-later or MIT) */
> +/*
> + * Author: David Heidelberg <david@ixit.cz>
> + */
> +
> +#ifndef _DT_BINDINGS_SMB347_CHARGER_H
> +#define _DT_BINDINGS_SMB347_CHARGER_H
> +
> +/* Charging compensation method */
> +#define SMB3XX_SOFT_TEMP_COMPENSATE_NONE	0
> +#define SMB3XX_SOFT_TEMP_COMPENSATE_CURRENT	1
> +#define SMB3XX_SOFT_TEMP_COMPENSATE_VOLTAGE	2
> +
> +/* Charging enable control */
> +#define SMB3XX_CHG_ENABLE_SW			0
> +#define SMB3XX_CHG_ENABLE_PIN_ACTIVE_LOW	1
> +#define SMB3XX_CHG_ENABLE_PIN_ACTIVE_HIGH	2
> +
> +#endif
> -- 
> 2.25.0
> 

Re: [PATCH 4/9] dt-bindings: power: supply: Add device-tree binding for Summit SMB3xx

[Dmitry Osipenko]

10.04.2020 19:49, Rob Herring пишет:
...
>> +  summit,max-chg-curr:
>> +    description: Maximum current for charging (in uA)
>> +    allOf:
>> +      - $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> +  summit,max-chg-volt:
>> +    description: Maximum voltage for charging (in uV)
>> +    allOf:
>> +      - $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 3500000
>> +    maximum: 4500000
>> +
>> +  summit,pre-chg-curr:
>> +    description: Pre-charging current for charging (in uA)
>> +    allOf:
>> +      - $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> +  summit,term-curr:
>> +    description: Charging cycle termination current (in uA)
>> +    allOf:
>> +      - $ref: /schemas/types.yaml#/definitions/uint32
...
> These are all properties of the battery attached and we have standard 
> properties for some/all of these.

Looks like only four properties seem to be matching the properties of
the battery.txt binding.

Are you suggesting that these matching properties should be renamed
after the properties in battery.txt?

> Also, any property with units should have a unit suffix as defined in 
> property-units.txt. And with that, you don't need to define the type.

Indeed, thank you for the suggestion.

Re: [PATCH 4/9] dt-bindings: power: supply: Add device-tree binding for Summit SMB3xx

[Rob Herring]

On Fri, Apr 10, 2020 at 2:02 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 10.04.2020 19:49, Rob Herring пишет:
> ...
> >> +  summit,max-chg-curr:
> >> +    description: Maximum current for charging (in uA)
> >> +    allOf:
> >> +      - $ref: /schemas/types.yaml#/definitions/uint32
> >> +
> >> +  summit,max-chg-volt:
> >> +    description: Maximum voltage for charging (in uV)
> >> +    allOf:
> >> +      - $ref: /schemas/types.yaml#/definitions/uint32
> >> +    minimum: 3500000
> >> +    maximum: 4500000
> >> +
> >> +  summit,pre-chg-curr:
> >> +    description: Pre-charging current for charging (in uA)
> >> +    allOf:
> >> +      - $ref: /schemas/types.yaml#/definitions/uint32
> >> +
> >> +  summit,term-curr:
> >> +    description: Charging cycle termination current (in uA)
> >> +    allOf:
> >> +      - $ref: /schemas/types.yaml#/definitions/uint32
> ...
> > These are all properties of the battery attached and we have standard
> > properties for some/all of these.
>
> Looks like only four properties seem to be matching the properties of
> the battery.txt binding.
>
> Are you suggesting that these matching properties should be renamed
> after the properties in battery.txt?

Yes, and that there should be a battery node. Possibly you should add
new properties battery.txt. It's curious that different properties are
needed. Ultimately, for a given battery technology I would expect
there's a fixed set of properties needed to describe how to charge
them. Perhaps some of these properties can just be derived from other
properties and folks are just picking what a specific charger wants.
Unfortunately, we have just a mess of stuff made up for each charger
out there. I don't have the time nor the experience in this area to do
much more than say do better.

Rob

Re: [PATCH 4/9] dt-bindings: power: supply: Add device-tree binding for Summit SMB3xx

[Dmitry Osipenko]

15.04.2020 17:27, Rob Herring пишет:
> On Fri, Apr 10, 2020 at 2:02 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> 10.04.2020 19:49, Rob Herring пишет:
>> ...
>>>> +  summit,max-chg-curr:
>>>> +    description: Maximum current for charging (in uA)
>>>> +    allOf:
>>>> +      - $ref: /schemas/types.yaml#/definitions/uint32
>>>> +
>>>> +  summit,max-chg-volt:
>>>> +    description: Maximum voltage for charging (in uV)
>>>> +    allOf:
>>>> +      - $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    minimum: 3500000
>>>> +    maximum: 4500000
>>>> +
>>>> +  summit,pre-chg-curr:
>>>> +    description: Pre-charging current for charging (in uA)
>>>> +    allOf:
>>>> +      - $ref: /schemas/types.yaml#/definitions/uint32
>>>> +
>>>> +  summit,term-curr:
>>>> +    description: Charging cycle termination current (in uA)
>>>> +    allOf:
>>>> +      - $ref: /schemas/types.yaml#/definitions/uint32
>> ...
>>> These are all properties of the battery attached and we have standard
>>> properties for some/all of these.
>>
>> Looks like only four properties seem to be matching the properties of
>> the battery.txt binding.
>>
>> Are you suggesting that these matching properties should be renamed
>> after the properties in battery.txt?
> 
> Yes, and that there should be a battery node.

Usually, it's a battery that has a phandle to the power-supply. Isn't it?

> Possibly you should add
> new properties battery.txt. It's curious that different properties are
> needed.

I guess it should be possible to make all these properties generic.

Sebastian, will you be okay if we will add all the required properties
to the power_supply_core?

> Ultimately, for a given battery technology I would expect
> there's a fixed set of properties needed to describe how to charge
> them.

Please notice that the charger doesn't "only charge" the battery,
usually it also supplies power to the whole device.

For example, when battery is fully-charged and charger is connected to
the power source (USB or mains), then battery may not draw any current
at all.

> Perhaps some of these properties can just be derived from other
> properties and folks are just picking what a specific charger wants.

Could be so, but I don't know for sure.

Even if some properties could be derived from the others, it won't hurt
if we will specify everything explicitly in the device-tree.

> Unfortunately, we have just a mess of stuff made up for each charger
> out there. I don't have the time nor the experience in this area to do
> much more than say do better.

I don't think it's a mess in the kernel. For example, it's common that
embedded controllers are exposed to the system as "just a battery",
while in fact it's a combined charger + battery controller and the
charger parameters just couldn't be changed by SW.

In a case of the Nexus 7 devices, the battery controller and charger
controller are two separate entities in the system. The battery
controller (bq27541) only monitors status of the battery (charge level,
temperature and etc).

Re: [PATCH 4/9] dt-bindings: power: supply: Add device-tree binding for Summit SMB3xx

[Sebastian Reichel]

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

Hi,

On Wed, Apr 15, 2020 at 06:30:02PM +0300, Dmitry Osipenko wrote:
> 15.04.2020 17:27, Rob Herring пишет:
> > On Fri, Apr 10, 2020 at 2:02 PM Dmitry Osipenko <digetx@gmail.com> wrote:
> >>
> >> 10.04.2020 19:49, Rob Herring пишет:
> >> ...
> >>>> +  summit,max-chg-curr:
> >>>> +    description: Maximum current for charging (in uA)
> >>>> +    allOf:
> >>>> +      - $ref: /schemas/types.yaml#/definitions/uint32
> >>>> +
> >>>> +  summit,max-chg-volt:
> >>>> +    description: Maximum voltage for charging (in uV)
> >>>> +    allOf:
> >>>> +      - $ref: /schemas/types.yaml#/definitions/uint32
> >>>> +    minimum: 3500000
> >>>> +    maximum: 4500000
> >>>> +
> >>>> +  summit,pre-chg-curr:
> >>>> +    description: Pre-charging current for charging (in uA)
> >>>> +    allOf:
> >>>> +      - $ref: /schemas/types.yaml#/definitions/uint32
> >>>> +
> >>>> +  summit,term-curr:
> >>>> +    description: Charging cycle termination current (in uA)
> >>>> +    allOf:
> >>>> +      - $ref: /schemas/types.yaml#/definitions/uint32
> >> ...
> >>> These are all properties of the battery attached and we have standard
> >>> properties for some/all of these.
> >>
> >> Looks like only four properties seem to be matching the properties of
> >> the battery.txt binding.
> >>
> >> Are you suggesting that these matching properties should be renamed
> >> after the properties in battery.txt?
> > 
> > Yes, and that there should be a battery node.
> 
> Usually, it's a battery that has a phandle to the power-supply. Isn't it?

There are two things: The infrastructure described by 
Documentation/devicetree/bindings/power/supply/power-supply.yaml is
used for telling the operating system, that a battery is charged
by some charger. This is done by adding a power-supplies = <&phandle>
in the battery fuel gauge node referencing the charger and probably
what you mean here.

Then we have the infrastructure described by 
Documentation/devicetree/bindings/power/supply/battery.txt, which
provides data about the battery cell. In an ideal world we would
have only smart batteries providing this data, but we don't live
in such a world. So what we currently have is a binding looking
like this:

bat: dumb-battery {
    compatible = "simple-battery";

    // data about battery cell(s)
};

fuel-gauge {
    // fuel-gauge specific data

    supplies = <&charger>;
    monitored-battery = <&bat>;
};

charger: charger {
    // charger specific data

    monitored-battery = <&bat>;
};

In an ideal world, charger should possibly reference fuel-gauge
node, which could provide combined data. Right now we do not have
the infrastructure for that, so it needs to directly reference
the simple-battery node.

> > Possibly you should add
> > new properties battery.txt. It's curious that different properties are
> > needed.
> 
> I guess it should be possible to make all these properties generic.
> 
> Sebastian, will you be okay if we will add all the required properties
> to the power_supply_core?

Extending battery.txt is possible when something is missing. As Rob
mentioned quite a few are already described, though:

summit,max-chg-curr => constant-charge-current-max-microamp
summit,max-chg-volt => constant-charge-voltage-max-microvolt
summit,pre-chg-curr => precharge-current-microamp
summit,term-curr => charge-term-current-microamp

I think at least the battery temperature limits are something, that
should be added to the generic code.

> > Ultimately, for a given battery technology I would expect
> > there's a fixed set of properties needed to describe how to charge
> > them.
> 
> Please notice that the charger doesn't "only charge" the battery,
> usually it also supplies power to the whole device.
> 
> For example, when battery is fully-charged and charger is connected to
> the power source (USB or mains), then battery may not draw any current
> at all.

It is also a question of how good the charging process should be.
Technically I can charge a single cell Li-ion battery without
knowing much, but it can reduce battery life and/or be very slow.
It might even be dangerous, if charging is done at high
temperatures. Also some of the properties in the battery binding
are not about charging, but about gauging. Some devices basically
have only options to measure voltage and voltage drop over a
resistor and everything else must be done by the operating system.

> > Perhaps some of these properties can just be derived from other
> > properties and folks are just picking what a specific charger wants.
> 
> Could be so, but I don't know for sure.

I don't think we have things, that can be derived with a reasonable
amount of effort in the existing simple-battery binding, except for
energy-full-design-microwatt-hours & charge-full-design-microamp-hours.

> Even if some properties could be derived from the others, it won't hurt
> if we will specify everything explicitly in the device-tree.
> 
> > Unfortunately, we have just a mess of stuff made up for each charger
> > out there. I don't have the time nor the experience in this area to do
> > much more than say do better.
>
> I don't think it's a mess in the kernel. For example, it's common that
> embedded controllers are exposed to the system as "just a battery",
> while in fact it's a combined charger + battery controller and the
> charger parameters just couldn't be changed by SW.

A good EC driver exposes a charger and a battery device, so that
userspace can easily identify if a charger is connected.

> In a case of the Nexus 7 devices, the battery controller and charger
> controller are two separate entities in the system. The battery
> controller (bq27541) only monitors status of the battery (charge level,
> temperature and etc).

-- Sebastian

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

Re: [PATCH 1/9] power: supply: smb347-charger: IRQSTAT_D is volatile

[Sebastian Reichel]

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

Hi,

On Sun, Mar 29, 2020 at 06:15:44PM +0200, David Heidelberg wrote:
> From: Dmitry Osipenko <digetx@gmail.com>
> 
> Fix failure when USB cable is connected:
> smb347 2-006a: reading IRQSTAT_D failed
> 
> Fixes: 1502cfe19bac ("smb347-charger: Fix battery status reporting logic for charger faults")
> 
> Tested-by: David Heidelberg <david@ixit.cz>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> Signed-off-by: David Heidelberg <david@ixit.cz>
> ---

Thanks, queued to power-supply's for-next branch.

-- Sebastian

>  drivers/power/supply/smb347-charger.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/power/supply/smb347-charger.c b/drivers/power/supply/smb347-charger.c
> index c1d124b8be0c..d102921b3ab2 100644
> --- a/drivers/power/supply/smb347-charger.c
> +++ b/drivers/power/supply/smb347-charger.c
> @@ -1138,6 +1138,7 @@ static bool smb347_volatile_reg(struct device *dev, unsigned int reg)
>  	switch (reg) {
>  	case IRQSTAT_A:
>  	case IRQSTAT_C:
> +	case IRQSTAT_D:
>  	case IRQSTAT_E:
>  	case IRQSTAT_F:
>  	case STAT_A:
> -- 
> 2.25.0
> 

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

Re: [PATCH 2/9] power: supply: smb347-charger: Add delay before getting IRQSTAT

[Sebastian Reichel]

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

Hi,

On Sun, Mar 29, 2020 at 06:15:45PM +0200, David Heidelberg wrote:
> This delay-fix is picked up from downstream driver,
> we measured that 25 - 35 ms delay ensure that we get required data.
> 
> Tested on SMB347 on Nexus 7 2012. Otherwise IRQSTAT_E fails to provide
> correct information.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> Signed-off-by: David Heidelberg <david@ixit.cz>
> ---

Thanks, queued.

-- Sebastian

>  drivers/power/supply/smb347-charger.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/power/supply/smb347-charger.c b/drivers/power/supply/smb347-charger.c
> index d102921b3ab2..f99026d81f2a 100644
> --- a/drivers/power/supply/smb347-charger.c
> +++ b/drivers/power/supply/smb347-charger.c
> @@ -8,6 +8,7 @@
>   *          Mika Westerberg <mika.westerberg@linux.intel.com>
>   */
>  
> +#include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/gpio.h>
>  #include <linux/kernel.h>
> @@ -708,6 +709,9 @@ static irqreturn_t smb347_interrupt(int irq, void *data)
>  	bool handled = false;
>  	int ret;
>  
> +	/* SMB347 it needs at least 20ms for setting IRQSTAT_E_*IN_UV_IRQ */
> +	usleep_range(25000, 35000);
> +
>  	ret = regmap_read(smb->regmap, STAT_C, &stat_c);
>  	if (ret < 0) {
>  		dev_warn(smb->dev, "reading STAT_C failed\n");
> -- 
> 2.25.0
> 

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

Re: [PATCH 3/9] power: supply: smb347-charger: Use resource-managed API

[Sebastian Reichel]

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

Hi,

On Sun, Mar 29, 2020 at 06:15:46PM +0200, David Heidelberg wrote:
> Simplify code, more convenient to use with Device Tree.
> 
> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
> Signed-off-by: David Heidelberg <david@ixit.cz>
> ---

Generally I like this change a lot, but it changes the removal
order, so that the IRQ is only free'd after power-supply has
already been unregistered. While it is disabled in the sm347 I
think it's better to keep the previous order to be on the safe
side.

This can easily achieved by having another patch moving from
power_supply_register to devm_power_supply_register, which will
further cleanup the code as a nice side-effect.

-- Sebastian

>  drivers/power/supply/smb347-charger.c | 45 +++++++++++++--------------
>  1 file changed, 21 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/power/supply/smb347-charger.c b/drivers/power/supply/smb347-charger.c
> index f99026d81f2a..4add9f64ba90 100644
> --- a/drivers/power/supply/smb347-charger.c
> +++ b/drivers/power/supply/smb347-charger.c
> @@ -836,21 +836,31 @@ static int smb347_irq_init(struct smb347_charger *smb,
>  			   struct i2c_client *client)
>  {
>  	const struct smb347_charger_platform_data *pdata = smb->pdata;
> -	int ret, irq = gpio_to_irq(pdata->irq_gpio);
> +	unsigned long irqflags = IRQF_ONESHOT;
> +	int ret;
>  
> -	ret = gpio_request_one(pdata->irq_gpio, GPIOF_IN, client->name);
> -	if (ret < 0)
> -		goto fail;
> +	/* Requesting GPIO for IRQ is only needed in non-DT way */
> +	if (!client->irq) {
> +		int irq = gpio_to_irq(pdata->irq_gpio);
> +
> +		ret = devm_gpio_request_one(smb->dev, pdata->irq_gpio,
> +					    GPIOF_IN, client->name);
> +		if (ret < 0)
> +			return ret;
> +
> +		irqflags |= IRQF_TRIGGER_FALLING;
> +		client->irq = irq;
> +	}
>  
> -	ret = request_threaded_irq(irq, NULL, smb347_interrupt,
> -				   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> -				   client->name, smb);
> +	ret = devm_request_threaded_irq(smb->dev, client->irq, NULL,
> +					smb347_interrupt, irqflags,
> +					client->name, smb);
>  	if (ret < 0)
> -		goto fail_gpio;
> +		return ret;
>  
>  	ret = smb347_set_writable(smb, true);
>  	if (ret < 0)
> -		goto fail_irq;
> +		return ret;
>  
>  	/*
>  	 * Configure the STAT output to be suitable for interrupts: disable
> @@ -860,20 +870,10 @@ static int smb347_irq_init(struct smb347_charger *smb,
>  				 CFG_STAT_ACTIVE_HIGH | CFG_STAT_DISABLED,
>  				 CFG_STAT_DISABLED);
>  	if (ret < 0)
> -		goto fail_readonly;
> +		client->irq = 0;
>  
>  	smb347_set_writable(smb, false);
> -	client->irq = irq;
> -	return 0;
>  
> -fail_readonly:
> -	smb347_set_writable(smb, false);
> -fail_irq:
> -	free_irq(irq, smb);
> -fail_gpio:
> -	gpio_free(pdata->irq_gpio);
> -fail:
> -	client->irq = 0;
>  	return ret;
>  }
>  
> @@ -1299,11 +1299,8 @@ static int smb347_remove(struct i2c_client *client)
>  {
>  	struct smb347_charger *smb = i2c_get_clientdata(client);
>  
> -	if (client->irq) {
> +	if (client->irq)
>  		smb347_irq_disable(smb);
> -		free_irq(client->irq, smb);
> -		gpio_free(smb->pdata->irq_gpio);
> -	}
>  
>  	power_supply_unregister(smb->battery);
>  	if (smb->pdata->use_usb)
> -- 
> 2.25.0
> 

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

Re: [PATCH 6/9] power: supply: smb347-charger: Support SMB345 and SMB358

[Sebastian Reichel]

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

Hi,

On Sun, Mar 29, 2020 at 06:21:25PM +0200, David Heidelberg wrote:
> Tested SMB345 on Nexus 7 2013. Works.
> 
> Based on:
> - https://patchwork.kernel.org/patch/4922431/
> - https://patchwork.ozlabs.org/patch/666877/
> 
> Signed-off-by: David Heidelberg <david@ixit.cz>
> ---

LGTM.

-- Sebastian

>  drivers/power/supply/Kconfig          |   6 +-
>  drivers/power/supply/smb347-charger.c | 109 ++++++++++++++------------
>  2 files changed, 62 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index f3424fdce341..2581ed8aff5a 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -598,12 +598,12 @@ config CHARGER_BQ25890
>  	  Say Y to enable support for the TI BQ25890 battery charger.
>  
>  config CHARGER_SMB347
> -	tristate "Summit Microelectronics SMB347 Battery Charger"
> +	tristate "Summit Microelectronics SMB3XX Battery Charger"
>  	depends on I2C
>  	select REGMAP_I2C
>  	help
> -	  Say Y to include support for Summit Microelectronics SMB347
> -	  Battery Charger.
> +	  Say Y to include support for Summit Microelectronics SMB345,
> +	  SMB347 or SMB358 Battery Charger.
>  
>  config CHARGER_TPS65090
>  	tristate "TPS65090 battery charger driver"
> diff --git a/drivers/power/supply/smb347-charger.c b/drivers/power/supply/smb347-charger.c
> index 852d2ab566e0..0cbd0743fd91 100644
> --- a/drivers/power/supply/smb347-charger.c
> +++ b/drivers/power/supply/smb347-charger.c
> @@ -128,6 +128,7 @@
>   * @mains: power_supply instance for AC/DC power
>   * @usb: power_supply instance for USB power
>   * @battery: power_supply instance for battery
> + * @id: SMB charger ID
>   * @mains_online: is AC/DC input connected
>   * @usb_online: is USB input connected
>   * @charging_enabled: is charging enabled
> @@ -140,64 +141,61 @@ struct smb347_charger {
>  	struct power_supply	*mains;
>  	struct power_supply	*usb;
>  	struct power_supply	*battery;
> +	unsigned int		id;
>  	bool			mains_online;
>  	bool			usb_online;
>  	bool			charging_enabled;
>  	const struct smb347_charger_platform_data *pdata;
>  };
>  
> -/* Fast charge current in uA */
> -static const unsigned int fcc_tbl[] = {
> -	700000,
> -	900000,
> -	1200000,
> -	1500000,
> -	1800000,
> -	2000000,
> -	2200000,
> -	2500000,
> +enum smb_charger_chipid {
> +	SMB345,
> +	SMB347,
> +	SMB358,
> +	NUM_CHIP_TYPES,
>  };
>  
> +/* Fast charge current in uA */
> +static const unsigned int fcc_tbl[NUM_CHIP_TYPES][8] = {
> +	[SMB345] = {  200000,  450000,  600000,  900000,
> +		     1300000, 1500000, 1800000, 2000000 },
> +	[SMB347] = {  700000,  900000, 1200000, 1500000,
> +		     1800000, 2000000, 2200000, 2500000 },
> +	[SMB358] = {  200000,  450000,  600000,  900000,
> +		     1300000, 1500000, 1800000, 2000000 },
> +};
>  /* Pre-charge current in uA */
> -static const unsigned int pcc_tbl[] = {
> -	100000,
> -	150000,
> -	200000,
> -	250000,
> +static const unsigned int pcc_tbl[NUM_CHIP_TYPES][4] = {
> +	[SMB345] = { 150000, 250000, 350000, 450000 },
> +	[SMB347] = { 100000, 150000, 200000, 250000 },
> +	[SMB358] = { 150000, 250000, 350000, 450000 },
>  };
>  
>  /* Termination current in uA */
> -static const unsigned int tc_tbl[] = {
> -	37500,
> -	50000,
> -	100000,
> -	150000,
> -	200000,
> -	250000,
> -	500000,
> -	600000,
> +static const unsigned int tc_tbl[NUM_CHIP_TYPES][8] = {
> +	[SMB345] = {  30000,  40000,  60000,  80000,
> +		     100000, 125000, 150000, 200000 },
> +	[SMB347] = {  37500,  50000, 100000, 150000,
> +		     200000, 250000, 500000, 600000 },
> +	[SMB358] = {  30000,  40000,  60000,  80000,
> +		     100000, 125000, 150000, 200000 },
>  };
>  
>  /* Input current limit in uA */
> -static const unsigned int icl_tbl[] = {
> -	300000,
> -	500000,
> -	700000,
> -	900000,
> -	1200000,
> -	1500000,
> -	1800000,
> -	2000000,
> -	2200000,
> -	2500000,
> +static const unsigned int icl_tbl[NUM_CHIP_TYPES][10] = {
> +	[SMB345] = {  300000,  500000,  700000, 1000000, 1500000,
> +		     1800000, 2000000, 2000000, 2000000, 2000000 },
> +	[SMB347] = {  300000,  500000,  700000,  900000, 1200000,
> +		     1500000, 1800000, 2000000, 2200000, 2500000 },
> +	[SMB358] = {  300000,  500000,  700000, 1000000, 1500000,
> +		     1800000, 2000000, 2000000, 2000000, 2000000 },
>  };
>  
>  /* Charge current compensation in uA */
> -static const unsigned int ccc_tbl[] = {
> -	250000,
> -	700000,
> -	900000,
> -	1200000,
> +static const unsigned int ccc_tbl[NUM_CHIP_TYPES][4] = {
> +	[SMB345] = {  200000,  450000,  600000,  900000 },
> +	[SMB347] = {  250000,  700000,  900000, 1200000 },
> +	[SMB358] = {  200000,  450000,  600000,  900000 },
>  };
>  
>  /* Convert register value to current using lookup table */
> @@ -352,10 +350,11 @@ static int smb347_start_stop_charging(struct smb347_charger *smb)
>  
>  static int smb347_set_charge_current(struct smb347_charger *smb)
>  {
> +	unsigned int id = smb->id;
>  	int ret;
>  
>  	if (smb->pdata->max_charge_current) {
> -		ret = current_to_hw(fcc_tbl, ARRAY_SIZE(fcc_tbl),
> +		ret = current_to_hw(fcc_tbl[id], ARRAY_SIZE(fcc_tbl[id]),
>  				    smb->pdata->max_charge_current);
>  		if (ret < 0)
>  			return ret;
> @@ -368,7 +367,7 @@ static int smb347_set_charge_current(struct smb347_charger *smb)
>  	}
>  
>  	if (smb->pdata->pre_charge_current) {
> -		ret = current_to_hw(pcc_tbl, ARRAY_SIZE(pcc_tbl),
> +		ret = current_to_hw(pcc_tbl[id], ARRAY_SIZE(pcc_tbl[id]),
>  				    smb->pdata->pre_charge_current);
>  		if (ret < 0)
>  			return ret;
> @@ -381,7 +380,7 @@ static int smb347_set_charge_current(struct smb347_charger *smb)
>  	}
>  
>  	if (smb->pdata->termination_current) {
> -		ret = current_to_hw(tc_tbl, ARRAY_SIZE(tc_tbl),
> +		ret = current_to_hw(tc_tbl[id], ARRAY_SIZE(tc_tbl[id]),
>  				    smb->pdata->termination_current);
>  		if (ret < 0)
>  			return ret;
> @@ -397,10 +396,11 @@ static int smb347_set_charge_current(struct smb347_charger *smb)
>  
>  static int smb347_set_current_limits(struct smb347_charger *smb)
>  {
> +	unsigned int id = smb->id;
>  	int ret;
>  
>  	if (smb->pdata->mains_current_limit) {
> -		ret = current_to_hw(icl_tbl, ARRAY_SIZE(icl_tbl),
> +		ret = current_to_hw(icl_tbl[id], ARRAY_SIZE(icl_tbl[id]),
>  				    smb->pdata->mains_current_limit);
>  		if (ret < 0)
>  			return ret;
> @@ -413,7 +413,7 @@ static int smb347_set_current_limits(struct smb347_charger *smb)
>  	}
>  
>  	if (smb->pdata->usb_hc_current_limit) {
> -		ret = current_to_hw(icl_tbl, ARRAY_SIZE(icl_tbl),
> +		ret = current_to_hw(icl_tbl[id], ARRAY_SIZE(icl_tbl[id]),
>  				    smb->pdata->usb_hc_current_limit);
>  		if (ret < 0)
>  			return ret;
> @@ -463,6 +463,7 @@ static int smb347_set_voltage_limits(struct smb347_charger *smb)
>  
>  static int smb347_set_temp_limits(struct smb347_charger *smb)
>  {
> +	unsigned int id = smb->id;
>  	bool enable_therm_monitor = false;
>  	int ret = 0;
>  	int val;
> @@ -587,7 +588,7 @@ static int smb347_set_temp_limits(struct smb347_charger *smb)
>  	}
>  
>  	if (smb->pdata->charge_current_compensation) {
> -		val = current_to_hw(ccc_tbl, ARRAY_SIZE(ccc_tbl),
> +		val = current_to_hw(ccc_tbl[id], ARRAY_SIZE(ccc_tbl[id]),
>  				    smb->pdata->charge_current_compensation);
>  		if (val < 0)
>  			return val;
> @@ -883,6 +884,7 @@ static int smb347_irq_init(struct smb347_charger *smb,
>   */
>  static int get_const_charge_current(struct smb347_charger *smb)
>  {
> +	unsigned int id = smb->id;
>  	int ret, intval;
>  	unsigned int v;
>  
> @@ -898,10 +900,12 @@ static int get_const_charge_current(struct smb347_charger *smb)
>  	 * and we can detect which table to use from bit 5.
>  	 */
>  	if (v & 0x20) {
> -		intval = hw_to_current(fcc_tbl, ARRAY_SIZE(fcc_tbl), v & 7);
> +		intval = hw_to_current(fcc_tbl[id],
> +				       ARRAY_SIZE(fcc_tbl[id]), v & 7);
>  	} else {
>  		v >>= 3;
> -		intval = hw_to_current(pcc_tbl, ARRAY_SIZE(pcc_tbl), v & 7);
> +		intval = hw_to_current(pcc_tbl[id],
> +				       ARRAY_SIZE(pcc_tbl[id]), v & 7);
>  	}
>  
>  	return intval;
> @@ -1317,6 +1321,7 @@ static int smb347_probe(struct i2c_client *client,
>  
>  	mutex_init(&smb->lock);
>  	smb->dev = &client->dev;
> +	smb->id = id->driver_data;
>  
>  	smb->regmap = devm_regmap_init_i2c(client, &smb347_regmap);
>  	if (IS_ERR(smb->regmap))
> @@ -1391,13 +1396,17 @@ static int smb347_remove(struct i2c_client *client)
>  }
>  
>  static const struct i2c_device_id smb347_id[] = {
> -	{ "smb347", 0 },
> -	{ }
> +	{ "smb345", SMB345 },
> +	{ "smb347", SMB347 },
> +	{ "smb358", SMB358 },
> +	{}
>  };
>  MODULE_DEVICE_TABLE(i2c, smb347_id);
>  
>  static const struct of_device_id smb3xx_of_match[] = {
> +	{ .compatible = "summit,smb345" },
>  	{ .compatible = "summit,smb347" },
> +	{ .compatible = "summit,smb358" },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, smb3xx_of_match);
> -- 
> 2.25.0
> 

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

Re: [PATCH 4/9] dt-bindings: power: supply: Add device-tree binding for Summit SMB3xx

[Dmitry Osipenko]

09.05.2020 04:14, Sebastian Reichel пишет:
> Hi,
> 
> On Wed, Apr 15, 2020 at 06:30:02PM +0300, Dmitry Osipenko wrote:
>> 15.04.2020 17:27, Rob Herring пишет:
>>> On Fri, Apr 10, 2020 at 2:02 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>>>>
>>>> 10.04.2020 19:49, Rob Herring пишет:
>>>> ...
>>>>>> +  summit,max-chg-curr:
>>>>>> +    description: Maximum current for charging (in uA)
>>>>>> +    allOf:
>>>>>> +      - $ref: /schemas/types.yaml#/definitions/uint32
>>>>>> +
>>>>>> +  summit,max-chg-volt:
>>>>>> +    description: Maximum voltage for charging (in uV)
>>>>>> +    allOf:
>>>>>> +      - $ref: /schemas/types.yaml#/definitions/uint32
>>>>>> +    minimum: 3500000
>>>>>> +    maximum: 4500000
>>>>>> +
>>>>>> +  summit,pre-chg-curr:
>>>>>> +    description: Pre-charging current for charging (in uA)
>>>>>> +    allOf:
>>>>>> +      - $ref: /schemas/types.yaml#/definitions/uint32
>>>>>> +
>>>>>> +  summit,term-curr:
>>>>>> +    description: Charging cycle termination current (in uA)
>>>>>> +    allOf:
>>>>>> +      - $ref: /schemas/types.yaml#/definitions/uint32
>>>> ...
>>>>> These are all properties of the battery attached and we have standard
>>>>> properties for some/all of these.
>>>>
>>>> Looks like only four properties seem to be matching the properties of
>>>> the battery.txt binding.
>>>>
>>>> Are you suggesting that these matching properties should be renamed
>>>> after the properties in battery.txt?
>>>
>>> Yes, and that there should be a battery node.
>>
>> Usually, it's a battery that has a phandle to the power-supply. Isn't it?
> 
> There are two things: The infrastructure described by 
> Documentation/devicetree/bindings/power/supply/power-supply.yaml is
> used for telling the operating system, that a battery is charged
> by some charger. This is done by adding a power-supplies = <&phandle>
> in the battery fuel gauge node referencing the charger and probably
> what you mean here.
> 
> Then we have the infrastructure described by 
> Documentation/devicetree/bindings/power/supply/battery.txt, which
> provides data about the battery cell. In an ideal world we would
> have only smart batteries providing this data, but we don't live
> in such a world. So what we currently have is a binding looking
> like this:
> 
> bat: dumb-battery {
>     compatible = "simple-battery";
> 
>     // data about battery cell(s)
> };
> 
> fuel-gauge {
>     // fuel-gauge specific data
> 
>     supplies = <&charger>;
>     monitored-battery = <&bat>;
> };
> 
> charger: charger {
>     // charger specific data
> 
>     monitored-battery = <&bat>;
> };
> 
> In an ideal world, charger should possibly reference fuel-gauge
> node, which could provide combined data. Right now we do not have
> the infrastructure for that, so it needs to directly reference
> the simple-battery node.
> 
>>> Possibly you should add
>>> new properties battery.txt. It's curious that different properties are
>>> needed.
>>
>> I guess it should be possible to make all these properties generic.
>>
>> Sebastian, will you be okay if we will add all the required properties
>> to the power_supply_core?
> 
> Extending battery.txt is possible when something is missing. As Rob
> mentioned quite a few are already described, though:
> 
> summit,max-chg-curr => constant-charge-current-max-microamp
> summit,max-chg-volt => constant-charge-voltage-max-microvolt
> summit,pre-chg-curr => precharge-current-microamp
> summit,term-curr => charge-term-current-microamp
> 
> I think at least the battery temperature limits are something, that
> should be added to the generic code.
> 
>>> Ultimately, for a given battery technology I would expect
>>> there's a fixed set of properties needed to describe how to charge
>>> them.
>>
>> Please notice that the charger doesn't "only charge" the battery,
>> usually it also supplies power to the whole device.
>>
>> For example, when battery is fully-charged and charger is connected to
>> the power source (USB or mains), then battery may not draw any current
>> at all.
> 
> It is also a question of how good the charging process should be.
> Technically I can charge a single cell Li-ion battery without
> knowing much, but it can reduce battery life and/or be very slow.
> It might even be dangerous, if charging is done at high
> temperatures. Also some of the properties in the battery binding
> are not about charging, but about gauging. Some devices basically
> have only options to measure voltage and voltage drop over a
> resistor and everything else must be done by the operating system.
> 
>>> Perhaps some of these properties can just be derived from other
>>> properties and folks are just picking what a specific charger wants.
>>
>> Could be so, but I don't know for sure.
> 
> I don't think we have things, that can be derived with a reasonable
> amount of effort in the existing simple-battery binding, except for
> energy-full-design-microwatt-hours & charge-full-design-microamp-hours.
> 
>> Even if some properties could be derived from the others, it won't hurt
>> if we will specify everything explicitly in the device-tree.
>>
>>> Unfortunately, we have just a mess of stuff made up for each charger
>>> out there. I don't have the time nor the experience in this area to do
>>> much more than say do better.
>>
>> I don't think it's a mess in the kernel. For example, it's common that
>> embedded controllers are exposed to the system as "just a battery",
>> while in fact it's a combined charger + battery controller and the
>> charger parameters just couldn't be changed by SW.
> 
> A good EC driver exposes a charger and a battery device, so that
> userspace can easily identify if a charger is connected.
> 
>> In a case of the Nexus 7 devices, the battery controller and charger
>> controller are two separate entities in the system. The battery
>> controller (bq27541) only monitors status of the battery (charge level,
>> temperature and etc).

Hello Sebastian,

Thank you very much for the comments! We'll prepare the v2.