[PATCH v4 0/2] power: supply: Add support for Maxim MAX8971 charger

[PATCH v4 0/2] power: supply: Add support for Maxim MAX8971 charger

[Svyatoslav Ryhel]

The MAX8971 is a compact, high-frequency, high-efficiency
switch-mode charger for a one-cell lithium-ion (Li+) battery.

---
Changes on switching from v3 to v4:
- swap graph with connector phandle

Changes on switching from v2 to v3:
- fast_charge_timer, top_off_threshold_current and top_off_timer converted to
  device attributes. Other vendor properties removed.
- removed max8971_config
- removed unneded functions and definitions along vendor props removal
- added __maybe_unused for resume function

Changes on switching from v1 to v2:
- swap phandle with graph for extcon
- added power-supply ref
---

Svyatoslav Ryhel (2):
  dt-bindings: power: supply: Document Maxim MAX8971 charger
  power: supply: Add support for Maxim MAX8971 charger

 .../bindings/power/supply/maxim,max8971.yaml  |  64 ++
 drivers/power/supply/Kconfig                  |  14 +
 drivers/power/supply/Makefile                 |   1 +
 drivers/power/supply/max8971_charger.c        | 759 ++++++++++++++++++
 4 files changed, 838 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/maxim,max8971.yaml
 create mode 100644 drivers/power/supply/max8971_charger.c

-- 
2.43.0

[PATCH v4 1/2] dt-bindings: power: supply: Document Maxim MAX8971 charger

[Svyatoslav Ryhel]

Add bindings for Maxim MAX8971 charger.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 .../bindings/power/supply/maxim,max8971.yaml  | 64 +++++++++++++++++++
 1 file changed, 64 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/maxim,max8971.yaml

diff --git a/Documentation/devicetree/bindings/power/supply/maxim,max8971.yaml b/Documentation/devicetree/bindings/power/supply/maxim,max8971.yaml
new file mode 100644
index 000000000000..d7b3e6ff6906
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/maxim,max8971.yaml
@@ -0,0 +1,64 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/supply/maxim,max8971.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Maxim MAX8971 IC charger
+
+maintainers:
+  - Svyatoslav Ryhel <clamor95@gmail.com>
+
+description:
+  The MAX8971 is a compact, high-frequency, high-efficiency switch-mode charger
+  for a one-cell lithium-ion (Li+) battery.
+
+allOf:
+  - $ref: power-supply.yaml#
+
+properties:
+  compatible:
+    const: maxim,max8971
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  monitored-battery: true
+
+  maxim,usb-connector:
+    description:
+      Phandle to a USB connector according to usb-connector.yaml. The connector
+      should be a child of the extcon device.
+    $ref: /schemas/types.yaml#/definitions/phandle
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        charger@35 {
+            compatible = "maxim,max8971";
+            reg = <0x35>;
+
+            interrupt-parent = <&gpio>;
+            interrupts = <74 IRQ_TYPE_LEVEL_LOW>;
+
+            monitored-battery = <&battery>;
+            maxim,usb-connector = <&usb_con>;
+        };
+    };
+...
-- 
2.43.0

[PATCH v4 2/2] power: supply: Add support for Maxim MAX8971 charger

[Svyatoslav Ryhel]

The MAX8971 is a compact, high-frequency, high-efficiency switch-mode
charger for a one-cell lithium-ion (Li+) battery.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/power/supply/Kconfig           |  14 +
 drivers/power/supply/Makefile          |   1 +
 drivers/power/supply/max8971_charger.c | 759 +++++++++++++++++++++++++
 3 files changed, 774 insertions(+)
 create mode 100644 drivers/power/supply/max8971_charger.c

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index 9f2eef6787f7..7c86116c2947 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -595,6 +595,20 @@ config CHARGER_MAX77976
 	  This driver can also be built as a module. If so, the module will be
 	  called max77976_charger.
 
+config CHARGER_MAX8971
+	tristate "Maxim MAX8971 battery charger driver"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  The MAX8971 is a compact, high-frequency, high-efficiency switch-mode
+	  charger for a one-cell lithium-ion (Li+) battery. It delivers up to
+	  1.55A of current to the battery from inputs up to 7.5V and withstands
+	  transient inputs up to 22V.
+
+	  Say Y to enable support for the Maxim MAX8971 battery charger.
+	  This driver can also be built as a module. If so, the module will be
+	  called max8971_charger.
+
 config CHARGER_MAX8997
 	tristate "Maxim MAX8997/MAX8966 PMIC battery charger driver"
 	depends on MFD_MAX8997 && REGULATOR_MAX8997
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index 59c4a9f40d28..8fa051ff87ff 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -81,6 +81,7 @@ obj-$(CONFIG_CHARGER_DETECTOR_MAX14656)	+= max14656_charger_detector.o
 obj-$(CONFIG_CHARGER_MAX77650)	+= max77650-charger.o
 obj-$(CONFIG_CHARGER_MAX77693)	+= max77693_charger.o
 obj-$(CONFIG_CHARGER_MAX77976)	+= max77976_charger.o
+obj-$(CONFIG_CHARGER_MAX8971)	+= max8971_charger.o
 obj-$(CONFIG_CHARGER_MAX8997)	+= max8997_charger.o
 obj-$(CONFIG_CHARGER_MAX8998)	+= max8998_charger.o
 obj-$(CONFIG_CHARGER_MP2629)	+= mp2629_charger.o
diff --git a/drivers/power/supply/max8971_charger.c b/drivers/power/supply/max8971_charger.c
new file mode 100644
index 000000000000..a271b1570e05
--- /dev/null
+++ b/drivers/power/supply/max8971_charger.c
@@ -0,0 +1,759 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+#include <linux/devm-helpers.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/extcon.h>
+#include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
+#include <linux/of_graph.h>
+#include <linux/property.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/power_supply.h>
+#include <linux/regmap.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>
+
+#define MAX8971_REG_CHGINT		0x0f
+#define   MAX8971_REG_CHG_RST		BIT(0)
+#define MAX8971_REG_CHGINT_MASK		0x01
+#define   MAX8971_AICL_MASK		BIT(7)
+#define MAX8971_REG_CHG_STAT		0x02
+#define   MAX8971_CHG_MASK		BIT(3)
+#define MAX8971_REG_DETAILS1		0x03
+#define MAX8971_REG_DETAILS2		0x04
+#define MAX8971_REG_CHGCNTL1		0x05
+#define MAX8971_REG_FCHGCRNT		0x06
+#define MAX8971_REG_DCCRNT		0x07
+#define   MAX8971_CHGRSTRT_MASK		BIT(6)
+#define MAX8971_REG_TOPOFF		0x08
+#define MAX8971_REG_TEMPREG		0x09
+#define MAX8971_REG_PROTCMD		0x0a
+#define   MAX8971_CHGPROT_LOCKED	0x00
+#define   MAX8971_CHGPROT_UNLOCKED	0x03
+
+#define MAX8971_FCHGT_DEFAULT		2
+#define MAX8971_TOPOFFT_DEFAULT		3
+
+static const char *max8971_manufacturer	= "Maxim Integrated";
+static const char *max8971_model	= "MAX8971";
+
+enum max8971_charging_state {
+	MAX8971_CHARGING_DEAD_BATTERY,
+	MAX8971_CHARGING_PREQUALIFICATION,
+	MAX8971_CHARGING_FAST_CONST_CURRENT,
+	MAX8971_CHARGING_FAST_CONST_VOLTAGE,
+	MAX8971_CHARGING_TOP_OFF,
+	MAX8971_CHARGING_DONE,
+	MAX8971_CHARGING_TIMER_FAULT,
+	MAX8971_CHARGING_SUSPENDED_THERMAL,
+	MAX8971_CHARGING_OFF,
+	MAX8971_CHARGING_THERMAL_LOOP,
+};
+
+enum max8971_health_state {
+	MAX8971_HEALTH_UNKNOWN,
+	MAX8971_HEALTH_COLD,
+	MAX8971_HEALTH_COOL,
+	MAX8971_HEALTH_WARM,
+	MAX8971_HEALTH_HOT,
+	MAX8971_HEALTH_OVERHEAT,
+};
+
+/* Fast-Charge current limit, 250..1550 mA, 50 mA steps */
+#define MAX8971_CHG_CC_STEP			  50000U
+#define MAX8971_CHG_CC_MIN			 250000U
+#define MAX8971_CHG_CC_MAX			1550000U
+
+/* Input current limit, 250..1500 mA, 25 mA steps */
+#define MAX8971_DCILMT_STEP			  25000U
+#define MAX8971_DCILMT_MIN			 250000U
+#define MAX8971_DCILMT_MAX			1500000U
+
+enum max8971_field_idx {
+	THM_DTLS,		/* DETAILS1 */
+	BAT_DTLS, CHG_DTLS,	/* DETAILS2 */
+	CHG_CC, FCHG_T,		/* FCHGCRNT */
+	DCI_LMT,		/* DCCRNT */
+	TOPOFF_T, TOPOFF_S,	/* TOPOFF */
+	CPROT,			/* PROTCMD */
+	MAX8971_N_REGMAP_FIELDS
+};
+
+static const struct reg_field max8971_reg_field[MAX8971_N_REGMAP_FIELDS] = {
+	[THM_DTLS] = REG_FIELD(MAX8971_REG_DETAILS1, 0, 2),
+	[BAT_DTLS] = REG_FIELD(MAX8971_REG_DETAILS2, 4, 5),
+	[CHG_DTLS] = REG_FIELD(MAX8971_REG_DETAILS2, 0, 3),
+	[CHG_CC]   = REG_FIELD(MAX8971_REG_FCHGCRNT, 0, 4),
+	[FCHG_T]   = REG_FIELD(MAX8971_REG_FCHGCRNT, 5, 7),
+	[DCI_LMT]  = REG_FIELD(MAX8971_REG_DCCRNT,   0, 5),
+	[TOPOFF_T] = REG_FIELD(MAX8971_REG_TOPOFF,   5, 7),
+	[TOPOFF_S] = REG_FIELD(MAX8971_REG_TOPOFF,   2, 3),
+	[CPROT]    = REG_FIELD(MAX8971_REG_PROTCMD,  2, 3),
+};
+
+static const struct regmap_config max8971_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = MAX8971_REG_CHGINT,
+};
+
+struct max8971_data {
+	struct i2c_client *client;
+	struct device *dev;
+	struct power_supply *psy_mains;
+
+	struct extcon_dev *edev;
+	struct notifier_block extcon_nb;
+	struct delayed_work extcon_work;
+
+	struct regmap *regmap;
+	struct regmap_field *rfield[MAX8971_N_REGMAP_FIELDS];
+
+	enum power_supply_usb_type usb_type;
+
+	u32 fchgt;
+	u32 tofft;
+	u32 toffs;
+
+	bool present;
+};
+
+static int max8971_get_status(struct max8971_data *priv, int *val)
+{
+	u32 regval;
+	int err;
+
+	err = regmap_field_read(priv->rfield[CHG_DTLS], &regval);
+	if (err)
+		return err;
+
+	switch (regval) {
+	case MAX8971_CHARGING_DEAD_BATTERY:
+	case MAX8971_CHARGING_PREQUALIFICATION:
+	case MAX8971_CHARGING_FAST_CONST_CURRENT:
+	case MAX8971_CHARGING_FAST_CONST_VOLTAGE:
+	case MAX8971_CHARGING_TOP_OFF:
+	case MAX8971_CHARGING_THERMAL_LOOP:
+		*val = POWER_SUPPLY_STATUS_CHARGING;
+		break;
+	case MAX8971_CHARGING_DONE:
+		*val = POWER_SUPPLY_STATUS_FULL;
+		break;
+	case MAX8971_CHARGING_TIMER_FAULT:
+		*val = POWER_SUPPLY_STATUS_NOT_CHARGING;
+		break;
+	case MAX8971_CHARGING_OFF:
+	case MAX8971_CHARGING_SUSPENDED_THERMAL:
+		*val = POWER_SUPPLY_STATUS_DISCHARGING;
+		break;
+	default:
+		*val = POWER_SUPPLY_STATUS_UNKNOWN;
+	}
+
+	return 0;
+}
+
+static int max8971_get_charge_type(struct max8971_data *priv, int *val)
+{
+	u32 regval;
+	int err;
+
+	err = regmap_field_read(priv->rfield[CHG_DTLS], &regval);
+	if (err)
+		return err;
+
+	switch (regval) {
+	case MAX8971_CHARGING_DEAD_BATTERY:
+	case MAX8971_CHARGING_PREQUALIFICATION:
+		*val = POWER_SUPPLY_CHARGE_TYPE_TRICKLE;
+		break;
+	case MAX8971_CHARGING_FAST_CONST_CURRENT:
+	case MAX8971_CHARGING_FAST_CONST_VOLTAGE:
+		*val = POWER_SUPPLY_CHARGE_TYPE_FAST;
+		break;
+	case MAX8971_CHARGING_TOP_OFF:
+	case MAX8971_CHARGING_THERMAL_LOOP:
+		*val = POWER_SUPPLY_CHARGE_TYPE_STANDARD;
+		break;
+	case MAX8971_CHARGING_DONE:
+	case MAX8971_CHARGING_TIMER_FAULT:
+	case MAX8971_CHARGING_SUSPENDED_THERMAL:
+	case MAX8971_CHARGING_OFF:
+		*val = POWER_SUPPLY_CHARGE_TYPE_NONE;
+		break;
+	default:
+		*val = POWER_SUPPLY_CHARGE_TYPE_UNKNOWN;
+	}
+
+	return 0;
+}
+
+static int max8971_get_health(struct max8971_data *priv, int *val)
+{
+	u32 regval;
+	int err;
+
+	err = regmap_field_read(priv->rfield[THM_DTLS], &regval);
+	if (err)
+		return err;
+
+	switch (regval) {
+	case MAX8971_HEALTH_COLD:
+		*val = POWER_SUPPLY_HEALTH_COLD;
+		break;
+	case MAX8971_HEALTH_COOL:
+		*val = POWER_SUPPLY_HEALTH_COOL;
+		break;
+	case MAX8971_HEALTH_WARM:
+		*val = POWER_SUPPLY_HEALTH_GOOD;
+		break;
+	case MAX8971_HEALTH_HOT:
+		*val = POWER_SUPPLY_HEALTH_HOT;
+		break;
+	case MAX8971_HEALTH_OVERHEAT:
+		*val = POWER_SUPPLY_HEALTH_OVERHEAT;
+		break;
+	case MAX8971_HEALTH_UNKNOWN:
+	default:
+		*val = POWER_SUPPLY_HEALTH_UNKNOWN;
+	}
+
+	return 0;
+}
+
+static int max8971_get_online(struct max8971_data *priv, int *val)
+{
+	u32 regval;
+	int err;
+
+	err = regmap_read(priv->regmap, MAX8971_REG_CHG_STAT, &regval);
+	if (err)
+		return err;
+
+	if (priv->present)
+		/* CHG_OK bit is 0 when charger is online */
+		*val = !(regval & MAX8971_CHG_MASK);
+	else
+		*val = priv->present;
+
+	return 0;
+}
+
+static int max8971_get_integer(struct max8971_data *priv, enum max8971_field_idx fidx,
+			       u32 clamp_min, u32 clamp_max, u32 mult, int *val)
+{
+	u32 regval;
+	int err;
+
+	err = regmap_field_read(priv->rfield[fidx], &regval);
+	if (err)
+		return err;
+
+	*val = clamp_val(regval * mult, clamp_min, clamp_max);
+
+	return 0;
+}
+
+static int max8971_set_integer(struct max8971_data *priv, enum max8971_field_idx fidx,
+			       u32 clamp_min, u32 clamp_max, u32 div, int val)
+{
+	u32 regval;
+
+	regval = clamp_val(val, clamp_min, clamp_max) / div;
+
+	return regmap_field_write(priv->rfield[fidx], regval);
+}
+
+static int max8971_get_property(struct power_supply *psy, enum power_supply_property psp,
+				union power_supply_propval *val)
+{
+	struct max8971_data *priv = power_supply_get_drvdata(psy);
+	int err = 0;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_STATUS:
+		err = max8971_get_status(priv, &val->intval);
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_TYPE:
+		err = max8971_get_charge_type(priv, &val->intval);
+		break;
+	case POWER_SUPPLY_PROP_USB_TYPE:
+		val->intval = priv->usb_type;
+		break;
+	case POWER_SUPPLY_PROP_HEALTH:
+		err = max8971_get_health(priv, &val->intval);
+		break;
+	case POWER_SUPPLY_PROP_ONLINE:
+		err = max8971_get_online(priv, &val->intval);
+		break;
+	case POWER_SUPPLY_PROP_PRESENT:
+		val->intval = priv->present;
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX:
+		val->intval = MAX8971_CHG_CC_MAX;
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT:
+		err = max8971_get_integer(priv, CHG_CC, MAX8971_CHG_CC_MIN, MAX8971_CHG_CC_MAX,
+					  MAX8971_CHG_CC_STEP, &val->intval);
+		break;
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		err = max8971_get_integer(priv, DCI_LMT, MAX8971_DCILMT_MIN, MAX8971_DCILMT_MAX,
+					  MAX8971_DCILMT_STEP, &val->intval);
+		break;
+	case POWER_SUPPLY_PROP_MODEL_NAME:
+		val->strval = max8971_model;
+		break;
+	case POWER_SUPPLY_PROP_MANUFACTURER:
+		val->strval = max8971_manufacturer;
+		break;
+	default:
+		err = -EINVAL;
+	}
+
+	return err;
+}
+
+static int max8971_set_property(struct power_supply *psy, enum power_supply_property psp,
+				const union power_supply_propval *val)
+{
+	struct max8971_data *priv = power_supply_get_drvdata(psy);
+	int err = 0;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT:
+		err = max8971_set_integer(priv, CHG_CC, MAX8971_CHG_CC_MIN, MAX8971_CHG_CC_MAX,
+					  MAX8971_CHG_CC_STEP, val->intval);
+		break;
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		err = max8971_set_integer(priv, DCI_LMT, MAX8971_DCILMT_MIN, MAX8971_DCILMT_MAX,
+					  MAX8971_DCILMT_STEP, val->intval);
+		break;
+	default:
+		err = -EINVAL;
+	}
+
+	return err;
+};
+
+static int max8971_property_is_writeable(struct power_supply *psy,
+					 enum power_supply_property psp)
+{
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT:
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static enum power_supply_property max8971_properties[] = {
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_CHARGE_TYPE,
+	POWER_SUPPLY_PROP_USB_TYPE,
+	POWER_SUPPLY_PROP_HEALTH,
+	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT,
+	POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX,
+	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
+	POWER_SUPPLY_PROP_MODEL_NAME,
+	POWER_SUPPLY_PROP_MANUFACTURER,
+};
+
+static const struct power_supply_desc max8971_charger_desc = {
+	.name = "charger",
+	.type = POWER_SUPPLY_TYPE_USB,
+	.usb_types = BIT(POWER_SUPPLY_USB_TYPE_UNKNOWN) |
+		     BIT(POWER_SUPPLY_USB_TYPE_SDP) |
+		     BIT(POWER_SUPPLY_USB_TYPE_DCP) |
+		     BIT(POWER_SUPPLY_USB_TYPE_CDP) |
+		     BIT(POWER_SUPPLY_USB_TYPE_ACA),
+	.properties = max8971_properties,
+	.num_properties = ARRAY_SIZE(max8971_properties),
+	.get_property = max8971_get_property,
+	.set_property = max8971_set_property,
+	.property_is_writeable = max8971_property_is_writeable,
+};
+
+static void max8971_update_config(struct max8971_data *priv)
+{
+	regmap_field_write(priv->rfield[CPROT], MAX8971_CHGPROT_UNLOCKED);
+
+	if (priv->fchgt != MAX8971_FCHGT_DEFAULT)
+		regmap_field_write(priv->rfield[FCHG_T], priv->fchgt);
+
+	regmap_write_bits(priv->regmap, MAX8971_REG_DCCRNT, MAX8971_CHGRSTRT_MASK,
+			  MAX8971_CHGRSTRT_MASK);
+
+	if (priv->tofft != MAX8971_TOPOFFT_DEFAULT)
+		regmap_field_write(priv->rfield[TOPOFF_T], priv->tofft);
+
+	if (priv->toffs)
+		regmap_field_write(priv->rfield[TOPOFF_S], priv->toffs);
+
+	regmap_field_write(priv->rfield[CPROT], MAX8971_CHGPROT_LOCKED);
+}
+
+static ssize_t fast_charge_timer_show(struct device *dev, struct device_attribute *attr,
+				      char *buf)
+{
+	struct max8971_data *priv = dev_get_drvdata(dev);
+	u32 regval;
+	int err;
+
+	err = regmap_field_read(priv->rfield[FCHG_T], &regval);
+	if (err)
+		return err;
+
+	switch (regval) {
+	case 0x1 ... 0x7:
+		/* Time if off by 3 hours comparing to value */
+		regval += 3;
+		break;
+	case 0x0:
+	default:
+		regval = 0;
+		break;
+	}
+
+	return sysfs_emit(buf, "%u\n", regval);
+}
+
+static ssize_t fast_charge_timer_store(struct device *dev, struct device_attribute *attr,
+				       const char *buf, size_t count)
+{
+	struct max8971_data *priv = dev_get_drvdata(dev);
+	unsigned long hours;
+	int val, err;
+
+	err = kstrtoul(buf, 10, &hours);
+	if (err)
+		return err;
+
+	val = hours - 3;
+	if (val <= 0 || val > 7)
+		priv->fchgt = 0;
+	else
+		priv->fchgt = val;
+
+	max8971_update_config(priv);
+
+	return count;
+}
+
+static ssize_t top_off_threshold_current_show(struct device *dev,
+					      struct device_attribute *attr,
+					      char *buf)
+{
+	struct max8971_data *priv = dev_get_drvdata(dev);
+	u32 regval, val;
+	int err;
+
+	err = regmap_field_read(priv->rfield[TOPOFF_S], &regval);
+	if (err)
+		return err;
+
+	/* 50uA start with 50uA step */
+	val = regval * 50 + 50;
+	val *= 1000;
+
+	return sysfs_emit(buf, "%u\n", val);
+}
+
+static ssize_t top_off_threshold_current_store(struct device *dev,
+					       struct device_attribute *attr,
+					       const char *buf, size_t count)
+{
+	struct max8971_data *priv = dev_get_drvdata(dev);
+	unsigned long uamp;
+	int err;
+
+	err = kstrtoul(buf, 10, &uamp);
+	if (err)
+		return err;
+
+	if (uamp < 50000 || uamp > 200000)
+		return -EINVAL;
+
+	priv->toffs = uamp / 50000 - 1;
+
+	max8971_update_config(priv);
+
+	return count;
+}
+
+static ssize_t top_off_timer_show(struct device *dev, struct device_attribute *attr,
+				  char *buf)
+{
+	struct max8971_data *priv = dev_get_drvdata(dev);
+	u32 regval;
+	int err;
+
+	err = regmap_field_read(priv->rfield[TOPOFF_T], &regval);
+	if (err)
+		return err;
+
+	/* 10 min intervals */
+	regval *= 10;
+
+	return sysfs_emit(buf, "%u\n", regval);
+}
+
+static ssize_t top_off_timer_store(struct device *dev, struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	struct max8971_data *priv = dev_get_drvdata(dev);
+	unsigned long minutes;
+	int err;
+
+	err = kstrtoul(buf, 10, &minutes);
+	if (err)
+		return err;
+
+	if (minutes > 70)
+		return -EINVAL;
+
+	priv->tofft = minutes / 10;
+
+	max8971_update_config(priv);
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(fast_charge_timer);
+static DEVICE_ATTR_RW(top_off_threshold_current);
+static DEVICE_ATTR_RW(top_off_timer);
+
+static struct attribute *max8971_attributes[] = {
+	&dev_attr_fast_charge_timer.attr,
+	&dev_attr_top_off_threshold_current.attr,
+	&dev_attr_top_off_timer.attr,
+	NULL
+};
+
+static const struct attribute_group max8971_attr_group = {
+	.attrs = max8971_attributes,
+};
+
+static void max8971_extcon_evt_worker(struct work_struct *work)
+{
+	struct max8971_data *priv =
+		container_of(work, struct max8971_data, extcon_work.work);
+	struct device *dev = priv->dev;
+	struct extcon_dev *edev = priv->edev;
+	u32 chgcc, dcilmt;
+
+	if (extcon_get_state(edev, EXTCON_CHG_USB_SDP) > 0) {
+		dev_dbg(dev, "USB SDP charger is connected\n");
+		priv->usb_type = POWER_SUPPLY_USB_TYPE_SDP;
+		chgcc = 500000;
+		dcilmt = 500000;
+	} else if (extcon_get_state(edev, EXTCON_USB) > 0) {
+		dev_dbg(dev, "USB charger is connected\n");
+		priv->usb_type = POWER_SUPPLY_USB_TYPE_SDP;
+		chgcc = 500000;
+		dcilmt = 500000;
+	} else if (extcon_get_state(edev, EXTCON_DISP_MHL) > 0) {
+		dev_dbg(dev, "MHL plug is connected\n");
+		priv->usb_type = POWER_SUPPLY_USB_TYPE_SDP;
+		chgcc = 500000;
+		dcilmt = 500000;
+	} else if (extcon_get_state(edev, EXTCON_CHG_USB_DCP) > 0) {
+		dev_dbg(dev, "USB DCP charger is connected\n");
+		priv->usb_type = POWER_SUPPLY_USB_TYPE_DCP;
+		chgcc = 900000;
+		dcilmt = 1200000;
+	} else if (extcon_get_state(edev, EXTCON_CHG_USB_FAST) > 0) {
+		dev_dbg(dev, "USB FAST charger is connected\n");
+		priv->usb_type = POWER_SUPPLY_USB_TYPE_ACA;
+		chgcc = 900000;
+		dcilmt = 1200000;
+	} else if (extcon_get_state(edev, EXTCON_CHG_USB_SLOW) > 0) {
+		dev_dbg(dev, "USB SLOW charger is connected\n");
+		priv->usb_type = POWER_SUPPLY_USB_TYPE_ACA;
+		chgcc = 900000;
+		dcilmt = 1200000;
+	} else if (extcon_get_state(edev, EXTCON_CHG_USB_CDP) > 0) {
+		dev_dbg(dev, "USB CDP charger is connected\n");
+		priv->usb_type = POWER_SUPPLY_USB_TYPE_CDP;
+		chgcc = 900000;
+		dcilmt = 1200000;
+	} else {
+		dev_dbg(dev, "USB state is unknown\n");
+		priv->usb_type = POWER_SUPPLY_USB_TYPE_UNKNOWN;
+		return;
+	}
+
+	regmap_field_write(priv->rfield[CPROT], MAX8971_CHGPROT_UNLOCKED);
+
+	max8971_set_integer(priv, CHG_CC, MAX8971_CHG_CC_MIN, MAX8971_CHG_CC_MAX,
+			    MAX8971_CHG_CC_STEP, chgcc);
+	max8971_set_integer(priv, DCI_LMT, MAX8971_DCILMT_MIN, MAX8971_DCILMT_MAX,
+			    MAX8971_DCILMT_STEP, dcilmt);
+
+	regmap_field_write(priv->rfield[CPROT], MAX8971_CHGPROT_LOCKED);
+}
+
+static int extcon_get_charger_type(struct notifier_block *nb,
+				   unsigned long state, void *data)
+{
+	struct max8971_data *priv =
+		container_of(nb, struct max8971_data, extcon_nb);
+	schedule_delayed_work(&priv->extcon_work, 0);
+	return NOTIFY_OK;
+}
+
+static irqreturn_t max8971_interrupt(int irq, void *dev_id)
+{
+	struct max8971_data *priv = dev_id;
+	struct device *dev = priv->dev;
+	int err, state;
+
+	err = regmap_read(priv->regmap, MAX8971_REG_CHGINT, &state);
+	if (err)
+		dev_err(dev, "interrupt reg read failed %d\n", err);
+
+	err = regmap_write_bits(priv->regmap, MAX8971_REG_CHGINT_MASK,
+				MAX8971_AICL_MASK, MAX8971_AICL_MASK);
+	if (err)
+		dev_err(dev, "failed to mask IRQ\n");
+
+	/* set presence prop */
+	priv->present = state & MAX8971_REG_CHG_RST;
+
+	/* on every plug chip resets to default */
+	if (priv->present)
+		max8971_update_config(priv);
+
+	/* update supply status */
+	power_supply_changed(priv->psy_mains);
+
+	return IRQ_HANDLED;
+}
+
+static char *max8971_supplied_to[] = {
+	"battery",
+};
+
+static int max8971_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct max8971_data *priv;
+	struct device_node *connector, *extcon;
+	struct power_supply_config cfg = { };
+	int err, i;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->client = client;
+	priv->dev = dev;
+	priv->usb_type = POWER_SUPPLY_USB_TYPE_UNKNOWN;
+
+	i2c_set_clientdata(client, priv);
+
+	priv->regmap = devm_regmap_init_i2c(client, &max8971_regmap_config);
+	if (IS_ERR(priv->regmap))
+		return dev_err_probe(dev, PTR_ERR(priv->regmap), "cannot allocate regmap\n");
+
+	for (i = 0; i < MAX8971_N_REGMAP_FIELDS; i++) {
+		priv->rfield[i] = devm_regmap_field_alloc(dev, priv->regmap, max8971_reg_field[i]);
+		if (IS_ERR(priv->rfield[i]))
+			return dev_err_probe(dev, PTR_ERR(priv->rfield[i]),
+					     "cannot allocate regmap field\n");
+	}
+
+	cfg.of_node = dev->of_node;
+	cfg.drv_data = priv;
+	cfg.supplied_to = max8971_supplied_to;
+	cfg.num_supplicants = ARRAY_SIZE(max8971_supplied_to);
+
+	priv->psy_mains = devm_power_supply_register(dev, &max8971_charger_desc, &cfg);
+	if (IS_ERR(priv->psy_mains))
+		return dev_err_probe(dev, PTR_ERR(priv->psy_mains),
+				     "failed to register mains supply\n");
+
+	err = regmap_write_bits(priv->regmap, MAX8971_REG_CHGINT_MASK, MAX8971_AICL_MASK,
+				MAX8971_AICL_MASK);
+	if (err)
+		return dev_err_probe(dev, err, "failed to mask IRQ\n");
+
+	err = devm_device_add_group(dev, &max8971_attr_group);
+	if (err)
+		return dev_err_probe(dev, err, "failed to create sysfs attributes\n");
+
+	err = devm_request_threaded_irq(dev, client->irq, NULL, &max8971_interrupt,
+					IRQF_ONESHOT | IRQF_SHARED, client->name, priv);
+	if (err)
+		return dev_err_probe(dev, err, "failed to register IRQ %d\n", client->irq);
+
+	/* Extcon support is not vital for the charger to work */
+	connector = of_parse_phandle(dev->of_node, "maxim,usb-connector", 0);
+	extcon = of_get_parent(connector);
+	of_node_put(connector);
+
+	priv->edev = extcon_find_edev_by_node(extcon);
+	of_node_put(extcon);
+	if (IS_ERR(priv->edev))
+		return 0;
+
+	err = devm_delayed_work_autocancel(dev, &priv->extcon_work,
+					   max8971_extcon_evt_worker);
+	if (err)
+		return dev_err_probe(dev, err, "failed to add extcon evt stop action\n");
+
+	priv->extcon_nb.notifier_call = extcon_get_charger_type;
+
+	err = devm_extcon_register_notifier_all(dev, priv->edev, &priv->extcon_nb);
+	if (err)
+		return dev_err_probe(dev, err, "failed to register notifier\n");
+
+	/* Initial configuration work with 1 sec delay */
+	schedule_delayed_work(&priv->extcon_work, msecs_to_jiffies(1000));
+
+	return 0;
+}
+
+static int __maybe_unused max8971_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct max8971_data *priv = i2c_get_clientdata(client);
+
+	irq_wake_thread(client->irq, priv);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(max8971_pm_ops, NULL, max8971_resume);
+
+static const struct of_device_id max8971_match_ids[] = {
+	{ .compatible = "maxim,max8971" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, max8971_match_ids);
+
+static const struct i2c_device_id max8971_i2c_id[] = {
+	{ "max8971" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, max8971_i2c_id);
+
+static struct i2c_driver max8971_driver = {
+	.driver = {
+		.name = "max8971-charger",
+		.of_match_table = max8971_match_ids,
+		.pm = &max8971_pm_ops,
+	},
+	.probe = max8971_probe,
+	.id_table = max8971_i2c_id,
+};
+module_i2c_driver(max8971_driver);
+
+MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
+MODULE_DESCRIPTION("MAX8971 Charger Driver");
+MODULE_LICENSE("GPL");
-- 
2.43.0

Re: [PATCH v4 1/2] dt-bindings: power: supply: Document Maxim MAX8971 charger

[Rob Herring]

On Mon, Mar 10, 2025 at 10:02:36AM +0200, Svyatoslav Ryhel wrote:
> Add bindings for Maxim MAX8971 charger.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  .../bindings/power/supply/maxim,max8971.yaml  | 64 +++++++++++++++++++
>  1 file changed, 64 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/supply/maxim,max8971.yaml
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/maxim,max8971.yaml b/Documentation/devicetree/bindings/power/supply/maxim,max8971.yaml
> new file mode 100644
> index 000000000000..d7b3e6ff6906
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/maxim,max8971.yaml
> @@ -0,0 +1,64 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/supply/maxim,max8971.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Maxim MAX8971 IC charger
> +
> +maintainers:
> +  - Svyatoslav Ryhel <clamor95@gmail.com>
> +
> +description:
> +  The MAX8971 is a compact, high-frequency, high-efficiency switch-mode charger
> +  for a one-cell lithium-ion (Li+) battery.
> +
> +allOf:
> +  - $ref: power-supply.yaml#
> +
> +properties:
> +  compatible:
> +    const: maxim,max8971
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  monitored-battery: true
> +
> +  maxim,usb-connector:

Just 'connector', so when we have a 3rd case, we don't have a 3rd 
vendor.

> +    description:
> +      Phandle to a USB connector according to usb-connector.yaml. The connector
> +      should be a child of the extcon device.

'extcon' is a Linuxism. Is there an actual requirement here that's not 
*current* Linux requirements (which could change)? I assume the 
requirement is to have vbus or some supply?

> +    $ref: /schemas/types.yaml#/definitions/phandle
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        charger@35 {
> +            compatible = "maxim,max8971";
> +            reg = <0x35>;
> +
> +            interrupt-parent = <&gpio>;
> +            interrupts = <74 IRQ_TYPE_LEVEL_LOW>;
> +
> +            monitored-battery = <&battery>;
> +            maxim,usb-connector = <&usb_con>;
> +        };
> +    };
> +...
> -- 
> 2.43.0
> 

Re: [PATCH v4 1/2] dt-bindings: power: supply: Document Maxim MAX8971 charger

[Svyatoslav Ryhel]

вт, 11 бер. 2025 р. о 21:37 Rob Herring <robh@kernel.org> пише:
>
> On Mon, Mar 10, 2025 at 10:02:36AM +0200, Svyatoslav Ryhel wrote:
> > Add bindings for Maxim MAX8971 charger.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> >  .../bindings/power/supply/maxim,max8971.yaml  | 64 +++++++++++++++++++
> >  1 file changed, 64 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/power/supply/maxim,max8971.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/power/supply/maxim,max8971.yaml b/Documentation/devicetree/bindings/power/supply/maxim,max8971.yaml
> > new file mode 100644
> > index 000000000000..d7b3e6ff6906
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/supply/maxim,max8971.yaml
> > @@ -0,0 +1,64 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/power/supply/maxim,max8971.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Maxim MAX8971 IC charger
> > +
> > +maintainers:
> > +  - Svyatoslav Ryhel <clamor95@gmail.com>
> > +
> > +description:
> > +  The MAX8971 is a compact, high-frequency, high-efficiency switch-mode charger
> > +  for a one-cell lithium-ion (Li+) battery.
> > +
> > +allOf:
> > +  - $ref: power-supply.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: maxim,max8971
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  monitored-battery: true
> > +
> > +  maxim,usb-connector:
>
> Just 'connector', so when we have a 3rd case, we don't have a 3rd
> vendor.
>

Please, please be explicit and specific, you could not tell me this in
v3, you could but you decided to fuck up v4 as well. So wise.
Additionally, if you want a generic 'connector' which can be
referenced as 'connector: true' then add one, ATM this is classified
under your own terms as 'vendor property' and needs a vendor prefix.

> > +    description:
> > +      Phandle to a USB connector according to usb-connector.yaml. The connector
> > +      should be a child of the extcon device.
>
> 'extcon' is a Linuxism. Is there an actual requirement here that's not
> *current* Linux requirements (which could change)? I assume the
> requirement is to have vbus or some supply?
>

Pardon me, this schema is part of Linux kernel, no? I have no clue why
you collectively decided to just ignore external connector detection
devices. Ignorance does not affect the fact that such devices exist.

And no, it does not need vbus not supply, it needs EXTCON

> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        charger@35 {
> > +            compatible = "maxim,max8971";
> > +            reg = <0x35>;
> > +
> > +            interrupt-parent = <&gpio>;
> > +            interrupts = <74 IRQ_TYPE_LEVEL_LOW>;
> > +
> > +            monitored-battery = <&battery>;
> > +            maxim,usb-connector = <&usb_con>;
> > +        };
> > +    };
> > +...
> > --
> > 2.43.0
> >

Re: [PATCH v4 2/2] power: supply: Add support for Maxim MAX8971 charger

[Sebastian Reichel]

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

Hi,

I have a couple of comments inline.

On Mon, Mar 10, 2025 at 10:02:37AM +0200, Svyatoslav Ryhel wrote:
> The MAX8971 is a compact, high-frequency, high-efficiency switch-mode
> charger for a one-cell lithium-ion (Li+) battery.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
> diff --git a/drivers/power/supply/max8971_charger.c b/drivers/power/supply/max8971_charger.c
> new file mode 100644
> index 000000000000..a271b1570e05
> --- /dev/null
> +++ b/drivers/power/supply/max8971_charger.c
> @@ -0,0 +1,759 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +#include <linux/devm-helpers.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/extcon.h>
> +#include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/of_graph.h>
> +#include <linux/property.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/pm.h>
> +#include <linux/power_supply.h>
> +#include <linux/regmap.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>
> +
> +#define MAX8971_REG_CHGINT		0x0f
> +#define   MAX8971_REG_CHG_RST		BIT(0)
> +#define MAX8971_REG_CHGINT_MASK		0x01
> +#define   MAX8971_AICL_MASK		BIT(7)
> +#define MAX8971_REG_CHG_STAT		0x02
> +#define   MAX8971_CHG_MASK		BIT(3)
> +#define MAX8971_REG_DETAILS1		0x03
> +#define MAX8971_REG_DETAILS2		0x04
> +#define MAX8971_REG_CHGCNTL1		0x05
> +#define MAX8971_REG_FCHGCRNT		0x06
> +#define MAX8971_REG_DCCRNT		0x07
> +#define   MAX8971_CHGRSTRT_MASK		BIT(6)
> +#define MAX8971_REG_TOPOFF		0x08
> +#define MAX8971_REG_TEMPREG		0x09
> +#define MAX8971_REG_PROTCMD		0x0a
> +#define   MAX8971_CHGPROT_LOCKED	0x00
> +#define   MAX8971_CHGPROT_UNLOCKED	0x03
> +
> +#define MAX8971_FCHGT_DEFAULT		2
> +#define MAX8971_TOPOFFT_DEFAULT		3
> +
> +static const char *max8971_manufacturer	= "Maxim Integrated";
> +static const char *max8971_model	= "MAX8971";
> +
> +enum max8971_charging_state {
> +	MAX8971_CHARGING_DEAD_BATTERY,
> +	MAX8971_CHARGING_PREQUALIFICATION,
> +	MAX8971_CHARGING_FAST_CONST_CURRENT,
> +	MAX8971_CHARGING_FAST_CONST_VOLTAGE,
> +	MAX8971_CHARGING_TOP_OFF,
> +	MAX8971_CHARGING_DONE,
> +	MAX8971_CHARGING_TIMER_FAULT,
> +	MAX8971_CHARGING_SUSPENDED_THERMAL,
> +	MAX8971_CHARGING_OFF,
> +	MAX8971_CHARGING_THERMAL_LOOP,
> +};
> +
> +enum max8971_health_state {
> +	MAX8971_HEALTH_UNKNOWN,
> +	MAX8971_HEALTH_COLD,
> +	MAX8971_HEALTH_COOL,
> +	MAX8971_HEALTH_WARM,
> +	MAX8971_HEALTH_HOT,
> +	MAX8971_HEALTH_OVERHEAT,
> +};
> +
> +/* Fast-Charge current limit, 250..1550 mA, 50 mA steps */
> +#define MAX8971_CHG_CC_STEP			  50000U
> +#define MAX8971_CHG_CC_MIN			 250000U
> +#define MAX8971_CHG_CC_MAX			1550000U
> +
> +/* Input current limit, 250..1500 mA, 25 mA steps */
> +#define MAX8971_DCILMT_STEP			  25000U
> +#define MAX8971_DCILMT_MIN			 250000U
> +#define MAX8971_DCILMT_MAX			1500000U
> +
> +enum max8971_field_idx {
> +	THM_DTLS,		/* DETAILS1 */
> +	BAT_DTLS, CHG_DTLS,	/* DETAILS2 */
> +	CHG_CC, FCHG_T,		/* FCHGCRNT */
> +	DCI_LMT,		/* DCCRNT */
> +	TOPOFF_T, TOPOFF_S,	/* TOPOFF */
> +	CPROT,			/* PROTCMD */
> +	MAX8971_N_REGMAP_FIELDS
> +};
> +
> +static const struct reg_field max8971_reg_field[MAX8971_N_REGMAP_FIELDS] = {
> +	[THM_DTLS] = REG_FIELD(MAX8971_REG_DETAILS1, 0, 2),
> +	[BAT_DTLS] = REG_FIELD(MAX8971_REG_DETAILS2, 4, 5),
> +	[CHG_DTLS] = REG_FIELD(MAX8971_REG_DETAILS2, 0, 3),
> +	[CHG_CC]   = REG_FIELD(MAX8971_REG_FCHGCRNT, 0, 4),
> +	[FCHG_T]   = REG_FIELD(MAX8971_REG_FCHGCRNT, 5, 7),
> +	[DCI_LMT]  = REG_FIELD(MAX8971_REG_DCCRNT,   0, 5),
> +	[TOPOFF_T] = REG_FIELD(MAX8971_REG_TOPOFF,   5, 7),
> +	[TOPOFF_S] = REG_FIELD(MAX8971_REG_TOPOFF,   2, 3),
> +	[CPROT]    = REG_FIELD(MAX8971_REG_PROTCMD,  2, 3),
> +};
> +
> +static const struct regmap_config max8971_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = MAX8971_REG_CHGINT,
> +};
> +
> +struct max8971_data {
> +	struct i2c_client *client;

This is never read and can be dropped.

> +	struct device *dev;
> +	struct power_supply *psy_mains;
> +
> +	struct extcon_dev *edev;
> +	struct notifier_block extcon_nb;
> +	struct delayed_work extcon_work;
> +
> +	struct regmap *regmap;
> +	struct regmap_field *rfield[MAX8971_N_REGMAP_FIELDS];
> +
> +	enum power_supply_usb_type usb_type;
> +
> +	u32 fchgt;
> +	u32 tofft;
> +	u32 toffs;
> +
> +	bool present;
> +};
> +
> +static int max8971_get_status(struct max8971_data *priv, int *val)
> +{
> +	u32 regval;
> +	int err;
> +
> +	err = regmap_field_read(priv->rfield[CHG_DTLS], &regval);
> +	if (err)
> +		return err;
> +
> +	switch (regval) {
> +	case MAX8971_CHARGING_DEAD_BATTERY:
> +	case MAX8971_CHARGING_PREQUALIFICATION:
> +	case MAX8971_CHARGING_FAST_CONST_CURRENT:
> +	case MAX8971_CHARGING_FAST_CONST_VOLTAGE:
> +	case MAX8971_CHARGING_TOP_OFF:
> +	case MAX8971_CHARGING_THERMAL_LOOP:
> +		*val = POWER_SUPPLY_STATUS_CHARGING;
> +		break;
> +	case MAX8971_CHARGING_DONE:
> +		*val = POWER_SUPPLY_STATUS_FULL;
> +		break;
> +	case MAX8971_CHARGING_TIMER_FAULT:
> +		*val = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +		break;
> +	case MAX8971_CHARGING_OFF:
> +	case MAX8971_CHARGING_SUSPENDED_THERMAL:
> +		*val = POWER_SUPPLY_STATUS_DISCHARGING;
> +		break;
> +	default:
> +		*val = POWER_SUPPLY_STATUS_UNKNOWN;
> +	}
> +
> +	return 0;
> +}
> +
> +static int max8971_get_charge_type(struct max8971_data *priv, int *val)
> +{
> +	u32 regval;
> +	int err;
> +
> +	err = regmap_field_read(priv->rfield[CHG_DTLS], &regval);
> +	if (err)
> +		return err;
> +
> +	switch (regval) {
> +	case MAX8971_CHARGING_DEAD_BATTERY:
> +	case MAX8971_CHARGING_PREQUALIFICATION:
> +		*val = POWER_SUPPLY_CHARGE_TYPE_TRICKLE;
> +		break;
> +	case MAX8971_CHARGING_FAST_CONST_CURRENT:
> +	case MAX8971_CHARGING_FAST_CONST_VOLTAGE:
> +		*val = POWER_SUPPLY_CHARGE_TYPE_FAST;
> +		break;
> +	case MAX8971_CHARGING_TOP_OFF:
> +	case MAX8971_CHARGING_THERMAL_LOOP:
> +		*val = POWER_SUPPLY_CHARGE_TYPE_STANDARD;
> +		break;
> +	case MAX8971_CHARGING_DONE:
> +	case MAX8971_CHARGING_TIMER_FAULT:
> +	case MAX8971_CHARGING_SUSPENDED_THERMAL:
> +	case MAX8971_CHARGING_OFF:
> +		*val = POWER_SUPPLY_CHARGE_TYPE_NONE;
> +		break;
> +	default:
> +		*val = POWER_SUPPLY_CHARGE_TYPE_UNKNOWN;
> +	}
> +
> +	return 0;
> +}
> +
> +static int max8971_get_health(struct max8971_data *priv, int *val)
> +{
> +	u32 regval;
> +	int err;
> +
> +	err = regmap_field_read(priv->rfield[THM_DTLS], &regval);
> +	if (err)
> +		return err;
> +
> +	switch (regval) {
> +	case MAX8971_HEALTH_COLD:
> +		*val = POWER_SUPPLY_HEALTH_COLD;
> +		break;
> +	case MAX8971_HEALTH_COOL:
> +		*val = POWER_SUPPLY_HEALTH_COOL;
> +		break;
> +	case MAX8971_HEALTH_WARM:
> +		*val = POWER_SUPPLY_HEALTH_GOOD;
> +		break;
> +	case MAX8971_HEALTH_HOT:
> +		*val = POWER_SUPPLY_HEALTH_HOT;
> +		break;
> +	case MAX8971_HEALTH_OVERHEAT:
> +		*val = POWER_SUPPLY_HEALTH_OVERHEAT;
> +		break;
> +	case MAX8971_HEALTH_UNKNOWN:
> +	default:
> +		*val = POWER_SUPPLY_HEALTH_UNKNOWN;
> +	}

I guess it makes sense to report POWER_SUPPLY_HEALTH_DEAD
for MAX8971_CHARGING_DEAD_BATTERY?

> +
> +	return 0;
> +}
> +
> +static int max8971_get_online(struct max8971_data *priv, int *val)
> +{
> +	u32 regval;
> +	int err;
> +
> +	err = regmap_read(priv->regmap, MAX8971_REG_CHG_STAT, &regval);
> +	if (err)
> +		return err;
> +
> +	if (priv->present)
> +		/* CHG_OK bit is 0 when charger is online */
> +		*val = !(regval & MAX8971_CHG_MASK);
> +	else
> +		*val = priv->present;
> +
> +	return 0;
> +}
> +
> +static int max8971_get_integer(struct max8971_data *priv, enum max8971_field_idx fidx,
> +			       u32 clamp_min, u32 clamp_max, u32 mult, int *val)
> +{
> +	u32 regval;
> +	int err;
> +
> +	err = regmap_field_read(priv->rfield[fidx], &regval);
> +	if (err)
> +		return err;
> +
> +	*val = clamp_val(regval * mult, clamp_min, clamp_max);
> +
> +	return 0;
> +}
> +
> +static int max8971_set_integer(struct max8971_data *priv, enum max8971_field_idx fidx,
> +			       u32 clamp_min, u32 clamp_max, u32 div, int val)
> +{
> +	u32 regval;
> +
> +	regval = clamp_val(val, clamp_min, clamp_max) / div;
> +
> +	return regmap_field_write(priv->rfield[fidx], regval);
> +}
> +
> +static int max8971_get_property(struct power_supply *psy, enum power_supply_property psp,
> +				union power_supply_propval *val)
> +{
> +	struct max8971_data *priv = power_supply_get_drvdata(psy);
> +	int err = 0;
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_STATUS:
> +		err = max8971_get_status(priv, &val->intval);
> +		break;
> +	case POWER_SUPPLY_PROP_CHARGE_TYPE:
> +		err = max8971_get_charge_type(priv, &val->intval);
> +		break;
> +	case POWER_SUPPLY_PROP_USB_TYPE:
> +		val->intval = priv->usb_type;
> +		break;
> +	case POWER_SUPPLY_PROP_HEALTH:
> +		err = max8971_get_health(priv, &val->intval);
> +		break;
> +	case POWER_SUPPLY_PROP_ONLINE:
> +		err = max8971_get_online(priv, &val->intval);
> +		break;
> +	case POWER_SUPPLY_PROP_PRESENT:
> +		val->intval = priv->present;
> +		break;
> +	case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX:
> +		val->intval = MAX8971_CHG_CC_MAX;
> +		break;
> +	case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT:
> +		err = max8971_get_integer(priv, CHG_CC, MAX8971_CHG_CC_MIN, MAX8971_CHG_CC_MAX,
> +					  MAX8971_CHG_CC_STEP, &val->intval);
> +		break;
> +	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> +		err = max8971_get_integer(priv, DCI_LMT, MAX8971_DCILMT_MIN, MAX8971_DCILMT_MAX,
> +					  MAX8971_DCILMT_STEP, &val->intval);
> +		break;
> +	case POWER_SUPPLY_PROP_MODEL_NAME:
> +		val->strval = max8971_model;
> +		break;
> +	case POWER_SUPPLY_PROP_MANUFACTURER:
> +		val->strval = max8971_manufacturer;
> +		break;
> +	default:
> +		err = -EINVAL;
> +	}
> +
> +	return err;
> +}
> +
> +static int max8971_set_property(struct power_supply *psy, enum power_supply_property psp,
> +				const union power_supply_propval *val)
> +{
> +	struct max8971_data *priv = power_supply_get_drvdata(psy);
> +	int err = 0;
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT:
> +		err = max8971_set_integer(priv, CHG_CC, MAX8971_CHG_CC_MIN, MAX8971_CHG_CC_MAX,
> +					  MAX8971_CHG_CC_STEP, val->intval);
> +		break;
> +	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> +		err = max8971_set_integer(priv, DCI_LMT, MAX8971_DCILMT_MIN, MAX8971_DCILMT_MAX,
> +					  MAX8971_DCILMT_STEP, val->intval);
> +		break;
> +	default:
> +		err = -EINVAL;
> +	}
> +
> +	return err;
> +};
> +
> +static int max8971_property_is_writeable(struct power_supply *psy,
> +					 enum power_supply_property psp)
> +{
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT:
> +	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static enum power_supply_property max8971_properties[] = {
> +	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_CHARGE_TYPE,
> +	POWER_SUPPLY_PROP_USB_TYPE,
> +	POWER_SUPPLY_PROP_HEALTH,
> +	POWER_SUPPLY_PROP_ONLINE,
> +	POWER_SUPPLY_PROP_PRESENT,
> +	POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT,
> +	POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX,
> +	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
> +	POWER_SUPPLY_PROP_MODEL_NAME,
> +	POWER_SUPPLY_PROP_MANUFACTURER,
> +};
> +
> +static const struct power_supply_desc max8971_charger_desc = {
> +	.name = "charger",
> +	.type = POWER_SUPPLY_TYPE_USB,
> +	.usb_types = BIT(POWER_SUPPLY_USB_TYPE_UNKNOWN) |
> +		     BIT(POWER_SUPPLY_USB_TYPE_SDP) |
> +		     BIT(POWER_SUPPLY_USB_TYPE_DCP) |
> +		     BIT(POWER_SUPPLY_USB_TYPE_CDP) |
> +		     BIT(POWER_SUPPLY_USB_TYPE_ACA),
> +	.properties = max8971_properties,
> +	.num_properties = ARRAY_SIZE(max8971_properties),
> +	.get_property = max8971_get_property,
> +	.set_property = max8971_set_property,
> +	.property_is_writeable = max8971_property_is_writeable,
> +};
> +
> +static void max8971_update_config(struct max8971_data *priv)
> +{
> +	regmap_field_write(priv->rfield[CPROT], MAX8971_CHGPROT_UNLOCKED);
> +
> +	if (priv->fchgt != MAX8971_FCHGT_DEFAULT)
> +		regmap_field_write(priv->rfield[FCHG_T], priv->fchgt);
> +
> +	regmap_write_bits(priv->regmap, MAX8971_REG_DCCRNT, MAX8971_CHGRSTRT_MASK,
> +			  MAX8971_CHGRSTRT_MASK);
> +
> +	if (priv->tofft != MAX8971_TOPOFFT_DEFAULT)
> +		regmap_field_write(priv->rfield[TOPOFF_T], priv->tofft);
> +
> +	if (priv->toffs)
> +		regmap_field_write(priv->rfield[TOPOFF_S], priv->toffs);
> +
> +	regmap_field_write(priv->rfield[CPROT], MAX8971_CHGPROT_LOCKED);
> +}
> +
> +static ssize_t fast_charge_timer_show(struct device *dev, struct device_attribute *attr,
> +				      char *buf)
> +{
> +	struct max8971_data *priv = dev_get_drvdata(dev);
> +	u32 regval;
> +	int err;
> +
> +	err = regmap_field_read(priv->rfield[FCHG_T], &regval);
> +	if (err)
> +		return err;
> +
> +	switch (regval) {
> +	case 0x1 ... 0x7:
> +		/* Time if off by 3 hours comparing to value */
> +		regval += 3;
> +		break;
> +	case 0x0:
> +	default:
> +		regval = 0;
> +		break;
> +	}
> +
> +	return sysfs_emit(buf, "%u\n", regval);
> +}
> +
> +static ssize_t fast_charge_timer_store(struct device *dev, struct device_attribute *attr,
> +				       const char *buf, size_t count)
> +{
> +	struct max8971_data *priv = dev_get_drvdata(dev);
> +	unsigned long hours;
> +	int val, err;
> +
> +	err = kstrtoul(buf, 10, &hours);
> +	if (err)
> +		return err;
> +
> +	val = hours - 3;
> +	if (val <= 0 || val > 7)
> +		priv->fchgt = 0;
> +	else
> +		priv->fchgt = val;
> +
> +	max8971_update_config(priv);
> +
> +	return count;
> +}
> +
> +static ssize_t top_off_threshold_current_show(struct device *dev,
> +					      struct device_attribute *attr,
> +					      char *buf)
> +{
> +	struct max8971_data *priv = dev_get_drvdata(dev);
> +	u32 regval, val;
> +	int err;
> +
> +	err = regmap_field_read(priv->rfield[TOPOFF_S], &regval);
> +	if (err)
> +		return err;
> +
> +	/* 50uA start with 50uA step */
> +	val = regval * 50 + 50;
> +	val *= 1000;
> +
> +	return sysfs_emit(buf, "%u\n", val);
> +}
> +
> +static ssize_t top_off_threshold_current_store(struct device *dev,
> +					       struct device_attribute *attr,
> +					       const char *buf, size_t count)
> +{
> +	struct max8971_data *priv = dev_get_drvdata(dev);
> +	unsigned long uamp;
> +	int err;
> +
> +	err = kstrtoul(buf, 10, &uamp);
> +	if (err)
> +		return err;
> +
> +	if (uamp < 50000 || uamp > 200000)
> +		return -EINVAL;
> +
> +	priv->toffs = uamp / 50000 - 1;
> +
> +	max8971_update_config(priv);
> +
> +	return count;
> +}
> +
> +static ssize_t top_off_timer_show(struct device *dev, struct device_attribute *attr,
> +				  char *buf)
> +{
> +	struct max8971_data *priv = dev_get_drvdata(dev);
> +	u32 regval;
> +	int err;
> +
> +	err = regmap_field_read(priv->rfield[TOPOFF_T], &regval);
> +	if (err)
> +		return err;
> +
> +	/* 10 min intervals */
> +	regval *= 10;
> +
> +	return sysfs_emit(buf, "%u\n", regval);
> +}
> +
> +static ssize_t top_off_timer_store(struct device *dev, struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	struct max8971_data *priv = dev_get_drvdata(dev);
> +	unsigned long minutes;
> +	int err;
> +
> +	err = kstrtoul(buf, 10, &minutes);
> +	if (err)
> +		return err;
> +
> +	if (minutes > 70)
> +		return -EINVAL;
> +
> +	priv->tofft = minutes / 10;
> +
> +	max8971_update_config(priv);
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(fast_charge_timer);
> +static DEVICE_ATTR_RW(top_off_threshold_current);
> +static DEVICE_ATTR_RW(top_off_timer);
> +
> +static struct attribute *max8971_attributes[] = {
> +	&dev_attr_fast_charge_timer.attr,
> +	&dev_attr_top_off_threshold_current.attr,
> +	&dev_attr_top_off_timer.attr,
> +	NULL
> +};

Missing ABI documentation. Also wondering if we can instead just
configure sensible values without exposing them to userspace. For
debugging things there is always the regmap debugfs API.

> +static const struct attribute_group max8971_attr_group = {
> +	.attrs = max8971_attributes,
> +};
> +
> +static void max8971_extcon_evt_worker(struct work_struct *work)
> +{
> +	struct max8971_data *priv =
> +		container_of(work, struct max8971_data, extcon_work.work);
> +	struct device *dev = priv->dev;
> +	struct extcon_dev *edev = priv->edev;
> +	u32 chgcc, dcilmt;
> +
> +	if (extcon_get_state(edev, EXTCON_CHG_USB_SDP) > 0) {
> +		dev_dbg(dev, "USB SDP charger is connected\n");
> +		priv->usb_type = POWER_SUPPLY_USB_TYPE_SDP;
> +		chgcc = 500000;
> +		dcilmt = 500000;
> +	} else if (extcon_get_state(edev, EXTCON_USB) > 0) {
> +		dev_dbg(dev, "USB charger is connected\n");
> +		priv->usb_type = POWER_SUPPLY_USB_TYPE_SDP;
> +		chgcc = 500000;
> +		dcilmt = 500000;
> +	} else if (extcon_get_state(edev, EXTCON_DISP_MHL) > 0) {
> +		dev_dbg(dev, "MHL plug is connected\n");
> +		priv->usb_type = POWER_SUPPLY_USB_TYPE_SDP;
> +		chgcc = 500000;
> +		dcilmt = 500000;
> +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_DCP) > 0) {
> +		dev_dbg(dev, "USB DCP charger is connected\n");
> +		priv->usb_type = POWER_SUPPLY_USB_TYPE_DCP;
> +		chgcc = 900000;
> +		dcilmt = 1200000;
> +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_FAST) > 0) {
> +		dev_dbg(dev, "USB FAST charger is connected\n");
> +		priv->usb_type = POWER_SUPPLY_USB_TYPE_ACA;
> +		chgcc = 900000;
> +		dcilmt = 1200000;
> +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_SLOW) > 0) {
> +		dev_dbg(dev, "USB SLOW charger is connected\n");
> +		priv->usb_type = POWER_SUPPLY_USB_TYPE_ACA;
> +		chgcc = 900000;
> +		dcilmt = 1200000;
> +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_CDP) > 0) {
> +		dev_dbg(dev, "USB CDP charger is connected\n");
> +		priv->usb_type = POWER_SUPPLY_USB_TYPE_CDP;
> +		chgcc = 900000;
> +		dcilmt = 1200000;
> +	} else {
> +		dev_dbg(dev, "USB state is unknown\n");
> +		priv->usb_type = POWER_SUPPLY_USB_TYPE_UNKNOWN;
> +		return;
> +	}
> +
> +	regmap_field_write(priv->rfield[CPROT], MAX8971_CHGPROT_UNLOCKED);
> +
> +	max8971_set_integer(priv, CHG_CC, MAX8971_CHG_CC_MIN, MAX8971_CHG_CC_MAX,
> +			    MAX8971_CHG_CC_STEP, chgcc);
> +	max8971_set_integer(priv, DCI_LMT, MAX8971_DCILMT_MIN, MAX8971_DCILMT_MAX,
> +			    MAX8971_DCILMT_STEP, dcilmt);
> +
> +	regmap_field_write(priv->rfield[CPROT], MAX8971_CHGPROT_LOCKED);
> +}
> +
> +static int extcon_get_charger_type(struct notifier_block *nb,
> +				   unsigned long state, void *data)
> +{
> +	struct max8971_data *priv =
> +		container_of(nb, struct max8971_data, extcon_nb);
> +	schedule_delayed_work(&priv->extcon_work, 0);
> +	return NOTIFY_OK;
> +}
> +
> +static irqreturn_t max8971_interrupt(int irq, void *dev_id)
> +{
> +	struct max8971_data *priv = dev_id;
> +	struct device *dev = priv->dev;
> +	int err, state;
> +
> +	err = regmap_read(priv->regmap, MAX8971_REG_CHGINT, &state);
> +	if (err)
> +		dev_err(dev, "interrupt reg read failed %d\n", err);
> +
> +	err = regmap_write_bits(priv->regmap, MAX8971_REG_CHGINT_MASK,
> +				MAX8971_AICL_MASK, MAX8971_AICL_MASK);
> +	if (err)
> +		dev_err(dev, "failed to mask IRQ\n");
> +
> +	/* set presence prop */
> +	priv->present = state & MAX8971_REG_CHG_RST;
> +
> +	/* on every plug chip resets to default */
> +	if (priv->present)
> +		max8971_update_config(priv);
> +
> +	/* update supply status */
> +	power_supply_changed(priv->psy_mains);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static char *max8971_supplied_to[] = {
> +	"battery",
> +};

This should not be needed for a DT based platform.

> +static int max8971_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct max8971_data *priv;
> +	struct device_node *connector, *extcon;
> +	struct power_supply_config cfg = { };
> +	int err, i;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->client = client;
> +	priv->dev = dev;
> +	priv->usb_type = POWER_SUPPLY_USB_TYPE_UNKNOWN;
> +
> +	i2c_set_clientdata(client, priv);
> +
> +	priv->regmap = devm_regmap_init_i2c(client, &max8971_regmap_config);
> +	if (IS_ERR(priv->regmap))
> +		return dev_err_probe(dev, PTR_ERR(priv->regmap), "cannot allocate regmap\n");
> +
> +	for (i = 0; i < MAX8971_N_REGMAP_FIELDS; i++) {
> +		priv->rfield[i] = devm_regmap_field_alloc(dev, priv->regmap, max8971_reg_field[i]);
> +		if (IS_ERR(priv->rfield[i]))
> +			return dev_err_probe(dev, PTR_ERR(priv->rfield[i]),
> +					     "cannot allocate regmap field\n");
> +	}
> +
> +	cfg.of_node = dev->of_node;

cfg.fwnode = dev_fwnode(dev);

> +	cfg.drv_data = priv;
> +	cfg.supplied_to = max8971_supplied_to;
> +	cfg.num_supplicants = ARRAY_SIZE(max8971_supplied_to);
> +
> +	priv->psy_mains = devm_power_supply_register(dev, &max8971_charger_desc, &cfg);
> +	if (IS_ERR(priv->psy_mains))
> +		return dev_err_probe(dev, PTR_ERR(priv->psy_mains),
> +				     "failed to register mains supply\n");
> +
> +	err = regmap_write_bits(priv->regmap, MAX8971_REG_CHGINT_MASK, MAX8971_AICL_MASK,
> +				MAX8971_AICL_MASK);
> +	if (err)
> +		return dev_err_probe(dev, err, "failed to mask IRQ\n");
> +
> +	err = devm_device_add_group(dev, &max8971_attr_group);
> +	if (err)
> +		return dev_err_probe(dev, err, "failed to create sysfs attributes\n");

Iff we need the custom properties they should be at least registered
automatically at device creation time via 'cfg.attr_grp'.

> +	err = devm_request_threaded_irq(dev, client->irq, NULL, &max8971_interrupt,
> +					IRQF_ONESHOT | IRQF_SHARED, client->name, priv);
> +	if (err)
> +		return dev_err_probe(dev, err, "failed to register IRQ %d\n", client->irq);
> +
> +	/* Extcon support is not vital for the charger to work */

The comment is a bit missleading, because the current code seems to
make extcon support mandatory as far as I can tell.

> +	connector = of_parse_phandle(dev->of_node, "maxim,usb-connector", 0);
> +	extcon = of_get_parent(connector);
> +	of_node_put(connector);
> +
> +	priv->edev = extcon_find_edev_by_node(extcon);
> +	of_node_put(extcon);
> +	if (IS_ERR(priv->edev))
> +		return 0;
> +
> +	err = devm_delayed_work_autocancel(dev, &priv->extcon_work,
> +					   max8971_extcon_evt_worker);
> +	if (err)
> +		return dev_err_probe(dev, err, "failed to add extcon evt stop action\n");
> +
> +	priv->extcon_nb.notifier_call = extcon_get_charger_type;
> +
> +	err = devm_extcon_register_notifier_all(dev, priv->edev, &priv->extcon_nb);
> +	if (err)
> +		return dev_err_probe(dev, err, "failed to register notifier\n");
> +
> +	/* Initial configuration work with 1 sec delay */
> +	schedule_delayed_work(&priv->extcon_work, msecs_to_jiffies(1000));
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused max8971_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct max8971_data *priv = i2c_get_clientdata(client);
> +
> +	irq_wake_thread(client->irq, priv);
> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(max8971_pm_ops, NULL, max8971_resume);
> +
> +static const struct of_device_id max8971_match_ids[] = {
> +	{ .compatible = "maxim,max8971" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, max8971_match_ids);
> +
> +static const struct i2c_device_id max8971_i2c_id[] = {
> +	{ "max8971" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, max8971_i2c_id);
> +
> +static struct i2c_driver max8971_driver = {
> +	.driver = {
> +		.name = "max8971-charger",
> +		.of_match_table = max8971_match_ids,
> +		.pm = &max8971_pm_ops,
> +	},
> +	.probe = max8971_probe,
> +	.id_table = max8971_i2c_id,
> +};
> +module_i2c_driver(max8971_driver);
> +
> +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> +MODULE_DESCRIPTION("MAX8971 Charger Driver");
> +MODULE_LICENSE("GPL");

Otherwise LGTM.

-- Sebastian

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

Re: [PATCH v4 2/2] power: supply: Add support for Maxim MAX8971 charger

[Svyatoslav Ryhel]

ср, 12 бер. 2025 р. о 10:50 Sebastian Reichel
<sebastian.reichel@collabora.com> пише:
>
> Hi,
>
> I have a couple of comments inline.
>
> On Mon, Mar 10, 2025 at 10:02:37AM +0200, Svyatoslav Ryhel wrote:
> > The MAX8971 is a compact, high-frequency, high-efficiency switch-mode
> > charger for a one-cell lithium-ion (Li+) battery.
> >
...
> > +
> > +static int max8971_get_health(struct max8971_data *priv, int *val)
> > +{
> > +     u32 regval;
> > +     int err;
> > +
> > +     err = regmap_field_read(priv->rfield[THM_DTLS], &regval);
> > +     if (err)
> > +             return err;
> > +
> > +     switch (regval) {
> > +     case MAX8971_HEALTH_COLD:
> > +             *val = POWER_SUPPLY_HEALTH_COLD;
> > +             break;
> > +     case MAX8971_HEALTH_COOL:
> > +             *val = POWER_SUPPLY_HEALTH_COOL;
> > +             break;
> > +     case MAX8971_HEALTH_WARM:
> > +             *val = POWER_SUPPLY_HEALTH_GOOD;
> > +             break;
> > +     case MAX8971_HEALTH_HOT:
> > +             *val = POWER_SUPPLY_HEALTH_HOT;
> > +             break;
> > +     case MAX8971_HEALTH_OVERHEAT:
> > +             *val = POWER_SUPPLY_HEALTH_OVERHEAT;
> > +             break;
> > +     case MAX8971_HEALTH_UNKNOWN:
> > +     default:
> > +             *val = POWER_SUPPLY_HEALTH_UNKNOWN;
> > +     }
>
> I guess it makes sense to report POWER_SUPPLY_HEALTH_DEAD
> for MAX8971_CHARGING_DEAD_BATTERY?
>

It seems that I have missed that, thank you for pointing.

> > +
> > +     return 0;
> > +}
> > +
...
> > +
> > +static DEVICE_ATTR_RW(fast_charge_timer);
> > +static DEVICE_ATTR_RW(top_off_threshold_current);
> > +static DEVICE_ATTR_RW(top_off_timer);
> > +
> > +static struct attribute *max8971_attributes[] = {
> > +     &dev_attr_fast_charge_timer.attr,
> > +     &dev_attr_top_off_threshold_current.attr,
> > +     &dev_attr_top_off_timer.attr,
> > +     NULL
> > +};
>
> Missing ABI documentation. Also wondering if we can instead just
> configure sensible values without exposing them to userspace. For
> debugging things there is always the regmap debugfs API.
>

These values are exposed like in the other maxim charger
(max77693_charger to be exact) so I inspired with that plus dt
maintainers were extremely against these in the tree.

Would be nice to have those configurable at least in some way, if not
by tree then by userspace. I assume ABI documentation should be a
separate patch.

> > +static const struct attribute_group max8971_attr_group = {
> > +     .attrs = max8971_attributes,
> > +};
> > +
...
> > +     err = devm_device_add_group(dev, &max8971_attr_group);
> > +     if (err)
> > +             return dev_err_probe(dev, err, "failed to create sysfs attributes\n");
>
> Iff we need the custom properties they should be at least registered
> automatically at device creation time via 'cfg.attr_grp'.
>

I actually did not know this was an option. Thanks for pointing out.

> > +     err = devm_request_threaded_irq(dev, client->irq, NULL, &max8971_interrupt,
> > +                                     IRQF_ONESHOT | IRQF_SHARED, client->name, priv);
> > +     if (err)
> > +             return dev_err_probe(dev, err, "failed to register IRQ %d\n", client->irq);
> > +
> > +     /* Extcon support is not vital for the charger to work */
>
> The comment is a bit missleading, because the current code seems to
> make extcon support mandatory as far as I can tell.
>

Extcon is optional and charger interrupt will work fine without it,
but this charger can only detect the fact of plug event, not the type
of plug. So without extcon charging will always be done like from usb2
pc port (default mode). Hence I have added extcon support here (which
my device has and uses) to be able to set higher current if a
dedicated charger is connected.

> > +     connector = of_parse_phandle(dev->of_node, "maxim,usb-connector", 0);
> > +     extcon = of_get_parent(connector);
> > +     of_node_put(connector);
> > +
> > +     priv->edev = extcon_find_edev_by_node(extcon);
> > +     of_node_put(extcon);
> > +     if (IS_ERR(priv->edev))
> > +             return 0;
> > +
> > +     err = devm_delayed_work_autocancel(dev, &priv->extcon_work,
> > +                                        max8971_extcon_evt_worker);
> > +     if (err)
> > +             return dev_err_probe(dev, err, "failed to add extcon evt stop action\n");
> > +
> > +     priv->extcon_nb.notifier_call = extcon_get_charger_type;
> > +
> > +     err = devm_extcon_register_notifier_all(dev, priv->edev, &priv->extcon_nb);
> > +     if (err)
> > +             return dev_err_probe(dev, err, "failed to register notifier\n");
> > +
> > +     /* Initial configuration work with 1 sec delay */
> > +     schedule_delayed_work(&priv->extcon_work, msecs_to_jiffies(1000));
> > +
> > +     return 0;
> > +}
> > +
> > +static int __maybe_unused max8971_resume(struct device *dev)
> > +{
> > +     struct i2c_client *client = to_i2c_client(dev);
> > +     struct max8971_data *priv = i2c_get_clientdata(client);
> > +
> > +     irq_wake_thread(client->irq, priv);
> > +
> > +     return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(max8971_pm_ops, NULL, max8971_resume);
> > +
> > +static const struct of_device_id max8971_match_ids[] = {
> > +     { .compatible = "maxim,max8971" },
> > +     { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, max8971_match_ids);
> > +
> > +static const struct i2c_device_id max8971_i2c_id[] = {
> > +     { "max8971" },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, max8971_i2c_id);
> > +
> > +static struct i2c_driver max8971_driver = {
> > +     .driver = {
> > +             .name = "max8971-charger",
> > +             .of_match_table = max8971_match_ids,
> > +             .pm = &max8971_pm_ops,
> > +     },
> > +     .probe = max8971_probe,
> > +     .id_table = max8971_i2c_id,
> > +};
> > +module_i2c_driver(max8971_driver);
> > +
> > +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> > +MODULE_DESCRIPTION("MAX8971 Charger Driver");
> > +MODULE_LICENSE("GPL");
>
> Otherwise LGTM.
>

Thank you for your suggestions, I will apply and test them.

> -- Sebastian

Re: [PATCH v4 1/2] dt-bindings: power: supply: Document Maxim MAX8971 charger

[Krzysztof Kozlowski]

On 12/03/2025 07:02, Svyatoslav Ryhel wrote:
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  monitored-battery: true
>>> +
>>> +  maxim,usb-connector:
>>
>> Just 'connector', so when we have a 3rd case, we don't have a 3rd
>> vendor.
>>
> 
> Please, please be explicit and specific, you could not tell me this in

git grep -C 3 connector:

> v3, you could but you decided to fuck up v4 as well. So wise.

We got a lot to review thus we make reviews concise. I understand that
it might lead to insufficient guidance, so more help in removing
workload from maintainers is always appreciated.

Instead of using vulgar words towards us, please put a bit more effort
and look at other recent bindings how they do it.

Review is provided in good faith and if it is by any chance incorrect,
it is enough to disagree instead of throwing things like above. That's
not acceptable.

> Additionally, if you want a generic 'connector' which can be
> referenced as 'connector: true' then add one, ATM this is classified
> under your own terms as 'vendor property' and needs a vendor prefix.

richtek,usb-connector is not the good example here. Your previous code here:
https://lore.kernel.org/all/20250225090014.59067-2-clamor95@gmail.com/

looks correct - you have there port. So where does charger_input point?


> 
>>> +    description:
>>> +      Phandle to a USB connector according to usb-connector.yaml. The connector
>>> +      should be a child of the extcon device.
>>
>> 'extcon' is a Linuxism. Is there an actual requirement here that's not
>> *current* Linux requirements (which could change)? I assume the
>> requirement is to have vbus or some supply?
>>
> 
> Pardon me, this schema is part of Linux kernel, no? I have no clue why

Bindings are used by other projects as well and they live here because
of possibility of review by skilled people and due to size of the
community. It does not make them, in general, Linux specific.

> you collectively decided to just ignore external connector detection
> devices. Ignorance does not affect the fact that such devices exist.

We didn't. They are described.

> 
> And no, it does not need vbus not supply, it needs EXTCON

There is no such thing as "extcon" from hardware point of view. Point us
to any standard or even wikipedia article describing it.


Best regards,
Krzysztof

Re: [PATCH v4 1/2] dt-bindings: power: supply: Document Maxim MAX8971 charger

[Svyatoslav Ryhel]

ср, 12 бер. 2025 р. о 11:49 Krzysztof Kozlowski <krzk@kernel.org> пише:
>
> On 12/03/2025 07:02, Svyatoslav Ryhel wrote:
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  interrupts:
> >>> +    maxItems: 1
> >>> +
> >>> +  monitored-battery: true
> >>> +
> >>> +  maxim,usb-connector:
> >>
> >> Just 'connector', so when we have a 3rd case, we don't have a 3rd
> >> vendor.
> >>
> >
> > Please, please be explicit and specific, you could not tell me this in
>
> git grep -C 3 connector:
>
> > v3, you could but you decided to fuck up v4 as well. So wise.
>
> We got a lot to review thus we make reviews concise. I understand that
> it might lead to insufficient guidance, so more help in removing
> workload from maintainers is always appreciated.
>
> Instead of using vulgar words towards us, please put a bit more effort
> and look at other recent bindings how they do it.
>
> Review is provided in good faith and if it is by any chance incorrect,
> it is enough to disagree instead of throwing things like above. That's
> not acceptable.
>
> > Additionally, if you want a generic 'connector' which can be
> > referenced as 'connector: true' then add one, ATM this is classified
> > under your own terms as 'vendor property' and needs a vendor prefix.
>
> richtek,usb-connector is not the good example here. Your previous code here:

Then what is a good example? This is the only example with binding Rob
requested,

> https://lore.kernel.org/all/20250225090014.59067-2-clamor95@gmail.com/
>
> looks correct - you have there port. So where does charger_input point?
>

It pointed to the port I have removed because Rob in v3 said it was
overkill and connector phandle was enough. May you resolve this inside
and not to contradict one another. Thank you.

>
> >
> >>> +    description:
> >>> +      Phandle to a USB connector according to usb-connector.yaml. The connector
> >>> +      should be a child of the extcon device.
> >>
> >> 'extcon' is a Linuxism. Is there an actual requirement here that's not
> >> *current* Linux requirements (which could change)? I assume the
> >> requirement is to have vbus or some supply?
> >>
> >
> > Pardon me, this schema is part of Linux kernel, no? I have no clue why
>
> Bindings are used by other projects as well and they live here because
> of possibility of review by skilled people and due to size of the
> community. It does not make them, in general, Linux specific.
>
> > you collectively decided to just ignore external connector detection
> > devices. Ignorance does not affect the fact that such devices exist.
>
> We didn't. They are described.
>
> >
> > And no, it does not need vbus not supply, it needs EXTCON
>
> There is no such thing as "extcon" from hardware point of view. Point us
> to any standard or even wikipedia article describing it.
>
>
> Best regards,
> Krzysztof

Re: [PATCH v4 1/2] dt-bindings: power: supply: Document Maxim MAX8971 charger

[Rob Herring]

On Wed, Mar 12, 2025 at 1:03 AM Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> вт, 11 бер. 2025 р. о 21:37 Rob Herring <robh@kernel.org> пише:
> >
> > On Mon, Mar 10, 2025 at 10:02:36AM +0200, Svyatoslav Ryhel wrote:
> > > Add bindings for Maxim MAX8971 charger.
> > >
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > ---
> > >  .../bindings/power/supply/maxim,max8971.yaml  | 64 +++++++++++++++++++
> > >  1 file changed, 64 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/power/supply/maxim,max8971.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/power/supply/maxim,max8971.yaml b/Documentation/devicetree/bindings/power/supply/maxim,max8971.yaml
> > > new file mode 100644
> > > index 000000000000..d7b3e6ff6906
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/power/supply/maxim,max8971.yaml
> > > @@ -0,0 +1,64 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/power/supply/maxim,max8971.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Maxim MAX8971 IC charger
> > > +
> > > +maintainers:
> > > +  - Svyatoslav Ryhel <clamor95@gmail.com>
> > > +
> > > +description:
> > > +  The MAX8971 is a compact, high-frequency, high-efficiency switch-mode charger
> > > +  for a one-cell lithium-ion (Li+) battery.
> > > +
> > > +allOf:
> > > +  - $ref: power-supply.yaml#
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: maxim,max8971
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  monitored-battery: true
> > > +
> > > +  maxim,usb-connector:
> >
> > Just 'connector', so when we have a 3rd case, we don't have a 3rd
> > vendor.
> >
>
> Please, please be explicit and specific, you could not tell me this in
> v3, you could but you decided to fuck up v4 as well. So wise.
> Additionally, if you want a generic 'connector' which can be
> referenced as 'connector: true' then add one, ATM this is classified
> under your own terms as 'vendor property' and needs a vendor prefix.

I don't care for your attitude, so I will be reviewing the 20+ other
bindings a day I have to review and not yours.

Rob

Re: [PATCH v4 2/2] power: supply: Add support for Maxim MAX8971 charger

[Svyatoslav Ryhel]

ср, 12 бер. 2025 р. о 11:15 Svyatoslav Ryhel <clamor95@gmail.com> пише:
>
> ср, 12 бер. 2025 р. о 10:50 Sebastian Reichel
> <sebastian.reichel@collabora.com> пише:
> >
> > Hi,
> >
> > I have a couple of comments inline.
> >
> > On Mon, Mar 10, 2025 at 10:02:37AM +0200, Svyatoslav Ryhel wrote:
> > > The MAX8971 is a compact, high-frequency, high-efficiency switch-mode
> > > charger for a one-cell lithium-ion (Li+) battery.
> > >
> ...
> > > +
> > > +static int max8971_get_health(struct max8971_data *priv, int *val)
> > > +{
> > > +     u32 regval;
> > > +     int err;
> > > +
> > > +     err = regmap_field_read(priv->rfield[THM_DTLS], &regval);
> > > +     if (err)
> > > +             return err;
> > > +
> > > +     switch (regval) {
> > > +     case MAX8971_HEALTH_COLD:
> > > +             *val = POWER_SUPPLY_HEALTH_COLD;
> > > +             break;
> > > +     case MAX8971_HEALTH_COOL:
> > > +             *val = POWER_SUPPLY_HEALTH_COOL;
> > > +             break;
> > > +     case MAX8971_HEALTH_WARM:
> > > +             *val = POWER_SUPPLY_HEALTH_GOOD;
> > > +             break;
> > > +     case MAX8971_HEALTH_HOT:
> > > +             *val = POWER_SUPPLY_HEALTH_HOT;
> > > +             break;
> > > +     case MAX8971_HEALTH_OVERHEAT:
> > > +             *val = POWER_SUPPLY_HEALTH_OVERHEAT;
> > > +             break;
> > > +     case MAX8971_HEALTH_UNKNOWN:
> > > +     default:
> > > +             *val = POWER_SUPPLY_HEALTH_UNKNOWN;
> > > +     }
> >
> > I guess it makes sense to report POWER_SUPPLY_HEALTH_DEAD
> > for MAX8971_CHARGING_DEAD_BATTERY?
> >
>
> It seems that I have missed that, thank you for pointing.
>

MAX8971_CHARGING_DEAD_BATTERY is not same as POWER_SUPPLY_HEALTH_DEAD
so we cannot use MAX8971_CHARGING_DEAD_BATTERY here. DEAD_BATTERY in
charging context is state of deep discharge not the battery health
overall. max8971_get_health returns charger state, not battery.
Battery state is monitored by dedicated controller.

> > > +
> > > +     return 0;
> > > +}
> > > +
> ...
> > > +
> > > +static DEVICE_ATTR_RW(fast_charge_timer);
> > > +static DEVICE_ATTR_RW(top_off_threshold_current);
> > > +static DEVICE_ATTR_RW(top_off_timer);
> > > +
> > > +static struct attribute *max8971_attributes[] = {
> > > +     &dev_attr_fast_charge_timer.attr,
> > > +     &dev_attr_top_off_threshold_current.attr,
> > > +     &dev_attr_top_off_timer.attr,
> > > +     NULL
> > > +};
> >
> > Missing ABI documentation. Also wondering if we can instead just
> > configure sensible values without exposing them to userspace. For
> > debugging things there is always the regmap debugfs API.
> >
>
> These values are exposed like in the other maxim charger
> (max77693_charger to be exact) so I inspired with that plus dt
> maintainers were extremely against these in the tree.
>
> Would be nice to have those configurable at least in some way, if not
> by tree then by userspace. I assume ABI documentation should be a
> separate patch.
>
> > > +static const struct attribute_group max8971_attr_group = {
> > > +     .attrs = max8971_attributes,
> > > +};
> > > +
> ...
> > > +     err = devm_device_add_group(dev, &max8971_attr_group);
> > > +     if (err)
> > > +             return dev_err_probe(dev, err, "failed to create sysfs attributes\n");
> >
> > Iff we need the custom properties they should be at least registered
> > automatically at device creation time via 'cfg.attr_grp'.
> >
>
> I actually did not know this was an option. Thanks for pointing out.
>
> > > +     err = devm_request_threaded_irq(dev, client->irq, NULL, &max8971_interrupt,
> > > +                                     IRQF_ONESHOT | IRQF_SHARED, client->name, priv);
> > > +     if (err)
> > > +             return dev_err_probe(dev, err, "failed to register IRQ %d\n", client->irq);
> > > +
> > > +     /* Extcon support is not vital for the charger to work */
> >
> > The comment is a bit missleading, because the current code seems to
> > make extcon support mandatory as far as I can tell.
> >
>
> Extcon is optional and charger interrupt will work fine without it,
> but this charger can only detect the fact of plug event, not the type
> of plug. So without extcon charging will always be done like from usb2
> pc port (default mode). Hence I have added extcon support here (which
> my device has and uses) to be able to set higher current if a
> dedicated charger is connected.
>
> > > +     connector = of_parse_phandle(dev->of_node, "maxim,usb-connector", 0);
> > > +     extcon = of_get_parent(connector);
> > > +     of_node_put(connector);
> > > +
> > > +     priv->edev = extcon_find_edev_by_node(extcon);
> > > +     of_node_put(extcon);
> > > +     if (IS_ERR(priv->edev))
> > > +             return 0;
> > > +
> > > +     err = devm_delayed_work_autocancel(dev, &priv->extcon_work,
> > > +                                        max8971_extcon_evt_worker);
> > > +     if (err)
> > > +             return dev_err_probe(dev, err, "failed to add extcon evt stop action\n");
> > > +
> > > +     priv->extcon_nb.notifier_call = extcon_get_charger_type;
> > > +
> > > +     err = devm_extcon_register_notifier_all(dev, priv->edev, &priv->extcon_nb);
> > > +     if (err)
> > > +             return dev_err_probe(dev, err, "failed to register notifier\n");
> > > +
> > > +     /* Initial configuration work with 1 sec delay */
> > > +     schedule_delayed_work(&priv->extcon_work, msecs_to_jiffies(1000));
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int __maybe_unused max8971_resume(struct device *dev)
> > > +{
> > > +     struct i2c_client *client = to_i2c_client(dev);
> > > +     struct max8971_data *priv = i2c_get_clientdata(client);
> > > +
> > > +     irq_wake_thread(client->irq, priv);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static SIMPLE_DEV_PM_OPS(max8971_pm_ops, NULL, max8971_resume);
> > > +
> > > +static const struct of_device_id max8971_match_ids[] = {
> > > +     { .compatible = "maxim,max8971" },
> > > +     { /* sentinel */ }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, max8971_match_ids);
> > > +
> > > +static const struct i2c_device_id max8971_i2c_id[] = {
> > > +     { "max8971" },
> > > +     { }
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, max8971_i2c_id);
> > > +
> > > +static struct i2c_driver max8971_driver = {
> > > +     .driver = {
> > > +             .name = "max8971-charger",
> > > +             .of_match_table = max8971_match_ids,
> > > +             .pm = &max8971_pm_ops,
> > > +     },
> > > +     .probe = max8971_probe,
> > > +     .id_table = max8971_i2c_id,
> > > +};
> > > +module_i2c_driver(max8971_driver);
> > > +
> > > +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> > > +MODULE_DESCRIPTION("MAX8971 Charger Driver");
> > > +MODULE_LICENSE("GPL");
> >
> > Otherwise LGTM.
> >
>
> Thank you for your suggestions, I will apply and test them.
>
> > -- Sebastian

Re: [PATCH v4 1/2] dt-bindings: power: supply: Document Maxim MAX8971 charger

[Svyatoslav Ryhel]

ср, 12 бер. 2025 р. о 11:49 Krzysztof Kozlowski <krzk@kernel.org> пише:
>
> On 12/03/2025 07:02, Svyatoslav Ryhel wrote:
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  interrupts:
> >>> +    maxItems: 1
> >>> +
> >>> +  monitored-battery: true
> >>> +
> >>> +  maxim,usb-connector:
> >>
> >> Just 'connector', so when we have a 3rd case, we don't have a 3rd
> >> vendor.
> >>
> >
> > Please, please be explicit and specific, you could not tell me this in
>
> git grep -C 3 connector:
>
> > v3, you could but you decided to fuck up v4 as well. So wise.
>
> We got a lot to review thus we make reviews concise. I understand that
> it might lead to insufficient guidance, so more help in removing
> workload from maintainers is always appreciated.
>
> Instead of using vulgar words towards us, please put a bit more effort
> and look at other recent bindings how they do it.
>
> Review is provided in good faith and if it is by any chance incorrect,
> it is enough to disagree instead of throwing things like above. That's
> not acceptable.
>
> > Additionally, if you want a generic 'connector' which can be
> > referenced as 'connector: true' then add one, ATM this is classified
> > under your own terms as 'vendor property' and needs a vendor prefix.
>
> richtek,usb-connector is not the good example here. Your previous code here:
> https://lore.kernel.org/all/20250225090014.59067-2-clamor95@gmail.com/
>
> looks correct - you have there port. So where does charger_input point?
>

Would the version from v3 with graph be acceptable?

>
> >
> >>> +    description:
> >>> +      Phandle to a USB connector according to usb-connector.yaml. The connector
> >>> +      should be a child of the extcon device.
> >>
> >> 'extcon' is a Linuxism. Is there an actual requirement here that's not
> >> *current* Linux requirements (which could change)? I assume the
> >> requirement is to have vbus or some supply?
> >>
> >
> > Pardon me, this schema is part of Linux kernel, no? I have no clue why
>
> Bindings are used by other projects as well and they live here because
> of possibility of review by skilled people and due to size of the
> community. It does not make them, in general, Linux specific.
>
> > you collectively decided to just ignore external connector detection
> > devices. Ignorance does not affect the fact that such devices exist.
>
> We didn't. They are described.
>
> >
> > And no, it does not need vbus not supply, it needs EXTCON
>
> There is no such thing as "extcon" from hardware point of view. Point us
> to any standard or even wikipedia article describing it.
>
>
> Best regards,
> Krzysztof